-
Notifications
You must be signed in to change notification settings - Fork 349
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: option to declare schema as const #1096
Merged
stephenh
merged 4 commits into
stephenh:main
from
cmd-johnson:feature/meta-typings-as-const
Sep 4, 2024
Merged
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
c10e4f4
feat: option to declare schema as const
cmd-johnson ba318c7
fix: don't write enum/service options when they're empty
cmd-johnson ffd4fd4
feat: merge outputSchemaAsConst flag into outputSchema flag
cmd-johnson 8dca741
style: fix formatting
cmd-johnson File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
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.
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.
Hi @cmd-johnson ! Wdyt of making
outputSchema
a tristate, likeoutputSchema=const
would enable this behavior, vs. a net-new flag? We've done that with a few other flags and it's seemed nice so far.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.
That's what I wanted to do at first, but
outputSchema
already can betrue
orno-file-descriptor
, which would mean that adding anas const
option would also require adding ano-file-descriptor-as-const
option or make the option's type an array of a newOutputSchema
enum (similar to howoutputServices
works) and I didn't want to update all usages ofno-file-descriptor
😅.If you'd rather go that route than adding another parameter, I can make those changes. I'd prefer going the enum array route in that case, because it scales better if more options get added to the
outputSchema
flag at some point in the future.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.
Ah yep! I was wondering if we'd run into that.
The enum array of
outputSchema=no-file-descriptor,const,etc
approach would be great, since you don't mind; thanks!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.
Done!
I've made it so you can combine the flags by specifying it multiple times, which is the way the
outputServices
flag works: