-
Notifications
You must be signed in to change notification settings - Fork 1
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
Implement printSchemaAndCapabilities, fix scalar type dependencies in schema response #29
Conversation
Update-sdk
…n connector and cli
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some general feedback on this PR, intended to help you make PRs that are more easily reviewable in the future 🙂
-
The PR title is not descriptive and is basically useless. What is this PR doing? Please at least rename this PR before merging.
-
This PR does not just do one thing. Instead it rolls up multiple different changes (some of which are mentioned in the description, some of which aren't):
- printSchemaAndCapabilities
- a fix to scalar types in schema
- switching from manual Display instances for errors to using thiserror
- create a release by bumping the version number
You should try to make one change per PR. Not only does it help you keep track of changes when you go back into your history, but it also helps the reviewer by cleanly separating changes out into separate reviewable pieces. It also makes reverting changes easier in the unlikely case that is required.
I'm happy to approve this, but please do make the requested minor changes before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this file? If this is just some file you used when testing locally, please delete it before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Removing
CHANGELOG.md
Outdated
@@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|||
## [Unreleased] | |||
|
|||
## [1.0.4] | |||
|
|||
- Implement PrintSchemaAndCapabilities command |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be more descriptive so that end users understand what this means better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some general feedback on this PR, intended to help you make PRs that are more easily reviewable in the future 🙂
The PR title is not descriptive and is basically useless. What is this PR doing? Please at least rename this PR before merging.
This PR does not just do one thing. Instead it rolls up multiple different changes (some of which are mentioned in the description, some of which aren't):
- printSchemaAndCapabilities
- a fix to scalar types in schema
- switching from manual Display instances for errors to using thiserror
- create a release by bumping the version number
You should try to make one change per PR. Not only does it help you keep track of changes when you go back into your history, but it also helps the reviewer by cleanly separating changes out into separate reviewable pieces. It also makes reverting changes easier in the unlikely case that is required.
I'm happy to approve this, but please do make the requested minor changes before merging.
RE: PR Title
That's 100% my bad. IDE picked up the last commit and used that for the name
Deleting that testing file, did not intend to commit it
This PR is a bit rushed because I needed to get that schema fix out ASAP, but duly noted on all points
Appreciate the feedback!
One thing I just noticed is that your changelog doesn't have dates on the versions, which means it technically isn't following "keep a changelog". Versions should look like this:
Also useful given your changelog update says the "Note the DDN CLI does not yet implement this"... knowing when this was written would be useful to readers. |
This PR implements the
printSchemaAndCapabilities
cli command.For this, we move the
schema_response
code into thecommon
crate. We also move the configuration reading code intocommon
This PR also includes a bugfix for schema response generation:
We try to only return scalar types actually used by the tables in the configuration.
Scalar types may have aggregate functions returning other scalar types, for example
sum(Int32)
returnsInt64
, creating scalar dependencies.These dependencies are now recursively accounted for.