Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: Multiple schema file support in schema add #3352
feat: Multiple schema file support in schema add #3352
Changes from 7 commits
74331e3
e4c55ad
746e13e
e006561
9cb6ecd
58e71f5
15b8d22
80a507b
a1740f4
88159c4
943173a
25f533d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
thought: IMO it would be nice to standardise this, so that all file-based inputs can be expected to work the same way.
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.
praise: This is a nice and simple way of ensure the whole operation takes place in a single txn.
thought: This does introduce a difference between the 3 clients - it might be nicer for users if instead we changed the other 2 to accept multiple strings. You would then be able to test this using integration tests instead of CLI specific stuff.
I think there is also an argument to be had that without changing the other 2 clients, the CLI should not have this feature, and instead users can just use
cat
/type
in their terminal.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.
todo: please move the errors to the
cli/errors.go
file.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.
todo: please move the errors to the
cli/errors.go
file.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.
todo: please move the errors to the
cli/errors.go
file.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.
thought: saying
strings
overstring
kind of signals to me that this is valid:defradb client schema add -f "schema1.graphql" "schema2.graphql"
rather than:
defradb client schema add -f "schema1.graphql" -f "schema2.graphql"
non-blocking: because maybe it might just be me
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.
I prefer your suggestion, but I have a vague memory of the team having this discussion and deciding on the multiple
-f
s. I hope I am wrong though :)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.
I have an opinion on this, but not a strong one. Happy to go with the team consensus (or majority opinion.)
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.
I don't have a strong prefer at all on the approach we take. My thought was only on the changed documentation in this PR, I think it should say
string
instead ofstrings
if we are going with the approach introduced in this PR (2.
above).Happy with
strings
terminology if we are switching to1.
above.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.
Oh, I see where you are coming from now. Hmm... I think I agree. I will be changing that to the singular as you suggest.
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.
Sorry commented on the main PR thread, can continue here if you like: #3352 (comment)