Skip to content

added getAllSchema function #76

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

Merged
merged 3 commits into from
Feb 6, 2025

Conversation

pavanydg
Copy link
Contributor

@pavanydg pavanydg commented Feb 3, 2025

I have added the getAllSchema function along with its types.
Also added test files for the function.

Copy link
Collaborator

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

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

Looks like a great start.

You have some linting issue you'll need to clean up. I suggest installing an ESLint plugin in your editor to get notified of these issues as you work so you can fix them immediately as they come up. Even then you should always run npm run lint before committing. Even I forget sometimes, but that's why it runs automatically when code is pushed.

I left a few notes inline. Please have a look.

I'm not sure about the name of the function (getAllSchemas). The function isn't actually returning schemas, it's returning schema identifiers. So, the name seems misleading. Also, I feel like it should say that they are "registered" schemas. In this implementation, schemas can come from the file system, or the web, or other places if configured to do so. The schemas being returned are only the schemas that are registered, not necessarily "all" schemas. That would make it something like getAllRegisteredSchemaUris. That seems a bit long. If you have another suggestion that meets the criteria I just described, then let me know, but it's ok if it ends up being a long name.

Last thing, make sure to add this function to the README. It doesn't need an example or anything, just add an entry in the API section. It can go next to the registerSchema and unregisterSchema entries.

@pavanydg
Copy link
Contributor Author

pavanydg commented Feb 5, 2025

closes #74

Copy link
Collaborator

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

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

Looks good! Just a couple minor things to clean up and it will be ready to go.

@jdesrosiers jdesrosiers merged commit ce4a88e into hyperjump-io:main Feb 6, 2025
2 checks passed
@jdesrosiers
Copy link
Collaborator

Thanks @pavanydg 🎉

@pavanydg
Copy link
Contributor Author

pavanydg commented Feb 6, 2025

Thank you for the guidance.

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