Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(resolve): change FetchSchema to support custom schema loaders #101

Merged
merged 3 commits into from
May 25, 2021
Merged

refactor(resolve): change FetchSchema to support custom schema loaders #101

merged 3 commits into from
May 25, 2021

Conversation

bmfs
Copy link
Contributor

@bmfs bmfs commented May 20, 2021

Added support for custom schema loaders based on the URI scheme

Some things to highlight:

  • Created a global loader registry similarly to the schemaRegistry and keywordRegistry
  • Converted http(s) and file to loader functions and they included by default on the global registry.

@bmfs
Copy link
Contributor Author

bmfs commented May 24, 2021

@Arqu let me know if anything is missing in the PR

Copy link
Contributor

@Arqu Arqu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bmfs thank you for the work on this, this is lovely, and cracks open some other issues that are outstanding.

Before merging, I'd require you to add some comments to respect the linter go lint and have everything pass there.

Specifically:

schema_loader.go:14:6: exported type LoaderRegistry should have comment or be unexported
schema_loader.go:18:6: exported type SchemaLoaderFunc should have comment or be unexported
schema_loader.go:20:1: exported function NewLoaderRegistry should have comment or be unexported

Also a nitpick I do have is the GetGlobalLoaderRegistry function naming, would rather it be just GetSchemaLoaderRegistry or GetLoaderRegistry, but if you feel strongly about highlighting this is specifically for the global registry, it's fine for now. In a future update where the thread safety is addressed this will probably be absorbed and changed.

Other than that, happy to merge as is and especially glad with the added flexibility.

@bmfs bmfs requested a review from Arqu May 24, 2021 16:08
Copy link
Contributor

@Arqu Arqu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@Arqu Arqu merged commit 36b0071 into qri-io:master May 25, 2021
@bmfs bmfs deleted the custom_uri_resolution branch May 25, 2021 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants