-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
[smart pipes] Add support for question-mark tokens #9209
Conversation
…, and tweak errors/comments
You might want to update The relevant code is at types.js/PipelinePrimaryTopicReference. |
Will do. Thanks for the heads up. |
On the other hand, it might be worth implementing generator support in a separate pull request, since, to my understanding, babel-generator is decoupled from babel-parser. I’ll work on it later today, in any case. |
How should the configuration option for generated topic tokens (default |
You can add a top-level option, like I did for decorators:
I understand that this is sub-optimal, but |
const pluginConfigurationTopicToken = this.getPluginOption( | ||
"pipelineOperator", | ||
"topicToken", | ||
); |
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.
Could you add a parameter to the getPluginOption
function for the default value? Then it would be enough to do
this.getPluginOption("pipelineOperator", "topicToken", "#") !== topicToken
and it wouldn't be necessary to use this separate function (that check could directly go in the if statement).
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.
Understood; I’ll replace this.
// Returns the value of the pipelineOperator plugin's topicToken configuration option. | ||
|
||
getPipelineOperatorPluginTopicTokenOption(): ?string { | ||
return this.getPluginOption("pipelineOperator", "topicToken"); |
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 think that having functions like this one is overkill: it has a name so long that it doesn't gain anything in readability neither in typing. Also, the name is basically the same as getPluginOption("pipelineOperator", "topicToken")
but with less punctation, it doesn't abstract anything more.
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.
Understood; I’ll inline this.
Yes, sounds good. Should I put this functionality in this pull request, or should I put it in a new pull request? |
Yes please! 👍 EDIT: I just noticed that "Yes" isn't an answer to your question lol. It is ok to put it in this pull request. |
Closing this as the smart pipeline proposal is not proposed. As of May 2021, the smart pipeline is replaced by hack pipeline. See also #13191. |
This pull request builds on #8289, which initially implemented parsing of the stage-0 smart-pipeline proposal. #8289 uses
#
as its “topic token” (aka the “placeholder token”), but this is only a preliminary choice, and the actual token has not yet been actually decided. This present pull request adds support for switching to the question mark?
as the topic token using a newtopicToken
configuration option on thepipelineOperator
parser plugin. Future pull requests will add support for other good possibilities, namely%
and@
.See also #9122 and #9179. CC @mAAdhaTTah, @littledan, @thiagoarrais.