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 to Package Catalog to Improve Readability #1595

Merged
merged 8 commits into from
May 23, 2022

Conversation

jakoguta
Copy link
Contributor

@jakoguta jakoguta commented May 9, 2022

This PR moves code around files in the package catalog to make it self documenting. It also adds comments to help better explain and document the code. This will make it more easy for new contributors to quickly understand the code and its implementation. There are also tests added to the package to ensure that as the code base evolves breaking changes are caught early in the unit tests.

NB: This refactor has only focused on tests and documentation.

Copy link
Collaborator

@kyleconroy kyleconroy left a comment

Choose a reason for hiding this comment

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

Thank you for adding comments. I don't think the testing changes are an improvement. sqlc is mainly tested using end-to-end tests. The benefit is that we can change the internal packages without having to write tests. I'd be happy to keep these tests if there ported over to the end-to-end package

@jakoguta
Copy link
Contributor Author

jakoguta commented May 9, 2022

This is well noted. I will move what is not already covered with end-to-end testing there.

@jakoguta jakoguta changed the title Refactor and Add Tests to Package Catalog Refactor to Package Catalog to Improve Readability May 12, 2022
@jakoguta jakoguta requested a review from kyleconroy May 12, 2022 05:30
@jakoguta
Copy link
Contributor Author

I have removed the unit tests in favor of endtoend tests

@kyleconroy
Copy link
Collaborator

Did you end up writing new endtoend tests? There aren't any in the changes.

Copy link
Collaborator

@kyleconroy kyleconroy 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 to me

@jakoguta
Copy link
Contributor Author

Did you end up writing new endtoend tests? There aren't any in the changes.

I am still working on what I want to add to the endtoend test.

@jakoguta jakoguta requested a review from kyleconroy May 13, 2022 10:30
@kyleconroy kyleconroy merged commit dfe4386 into sqlc-dev:main May 23, 2022
@jakoguta jakoguta mentioned this pull request Jul 8, 2022
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