Combine the 2 experimental RBS options into a single option #8863
Conversation
core/errors/internal.h
Outdated
| @@ -7,6 +7,7 @@ constexpr ErrorClass InternalError{1001, StrictLevel::Internal}; | |||
| // constexpr ErrorClass WrongSigil{1002, StrictLevel::Internal}; | |||
| constexpr ErrorClass CyclicReferenceError{1003, StrictLevel::Internal}; | |||
| constexpr ErrorClass FileNotFound{1004, StrictLevel::Internal}; | |||
| constexpr ErrorClass DeprecatedOptionError{1005, StrictLevel::Internal}; | |||
There was a problem hiding this comment.
We typically just let the option parser report errors, or otherwise provide the error directly in the options.cc option parsing code.
What do you think about not adding this ErrorClass and handling it during option parsing instead?
This also means that we can delete the fields from the CacheSensitiveOptions struct.
There was a problem hiding this comment.
Thanks, this is better indeed. I went with a logger->warn. Lmk if this is what you had in mind.
main/options/options.cc
Outdated
| @@ -458,6 +458,8 @@ buildOptions(const vector<pipeline::semantic_extension::SemanticExtensionProvide | |||
| "Enable experimental support for RBS signatures as inline comments"); | |||
| options.add_options(section)("enable-experimental-rbs-assertions", | |||
| "Enable experimental support for RBS assertions as inline comments"); | |||
| options.add_options(section)("enable-experimental-rbs", | |||
There was a problem hiding this comment.
Can we call this option --enable-experimental-rbs-comments?
There was a problem hiding this comment.
I don't want to give people the idea that this turns on support for reading RBS files, which it doesn't.
fe4e31a to
8ce4ec5
Compare
| @@ -154,6 +154,9 @@ Usage: | |||
| --enable-experimental-rbs-assertions | |||
There was a problem hiding this comment.
I assume this is being picked up because we still use raw["enable-experimental-rbs-assertions"] in options.cc? It'd be nice to mention deprecated here but I think it's fine.
There was a problem hiding this comment.
Yes, though it's not the usage (raw["enable-experimental-rbs-assertions"]) that matters, it's the definition (options.add_options). If you want to add a deprecation message, you would add it to the help message on that option.
But this was an experimental feature to begin with. I have no problem making unannounced breaking changes—up to and including simply deleting the options without warning, e.g. if you had chosen to implement this change by simply deleting the option and not adding a custom logger->warn message that would have been fine with me.
There was a problem hiding this comment.
I have only one suggested change. I'm going to accept it, kick off your build and enable auto-merge.
If you'd prefer to address the other comment (w.r.t. cli-ref and calling out deprecated) you can push to the branch an interrupt the build, or wait and make a follow up PR.
| @@ -154,6 +154,9 @@ Usage: | |||
| --enable-experimental-rbs-assertions | |||
There was a problem hiding this comment.
Yes, though it's not the usage (raw["enable-experimental-rbs-assertions"]) that matters, it's the definition (options.add_options). If you want to add a deprecation message, you would add it to the help message on that option.
But this was an experimental feature to begin with. I have no problem making unannounced breaking changes—up to and including simply deleting the options without warning, e.g. if you had chosen to implement this change by simply deleting the option and not adding a custom logger->warn message that would have been fine with me.
website/docs/error-reference.md
Outdated
| ## 1005 | ||
|
|
||
| Sorbet encountered a deprecated CLI flag. Change the supplied command or your | ||
| `sorbet/config` to use the appropriate flag mentioned in the error message. | ||
|
|
There was a problem hiding this comment.
| ## 1005 | |
| Sorbet encountered a deprecated CLI flag. Change the supplied command or your | |
| `sorbet/config` to use the appropriate flag mentioned in the error message. |
|
Update: I do not have permission to commit changes to your branch, so the ball is in your court actually. |
8ce4ec5 to
726fe1b
Compare
Deprecates
--enable-experimental-rbs-signaturesand--enable-experimental-rbs-assertionsin favour of--enable-experimental-rbs-commentsMotivation
We no longer think there's a benefit to having both options and it makes the pipeline code and adoption simpler. For now the old options are useable but give a deprecated error message instead of refusing to type check.
Test plan
See included automated tests.