Fix modern documentation to mention emit_arg_inside_procarg0 #658
Conversation
- Fix documentation of modern AST format to include emit_arg_inside_procarg0. This allows users to mirror what `Parser::Builders::Default.modernize` does.
|
BTW I think it would also make sense to add a public API for the effect of I'm happy to do so in a followup. |
|
@whitequark, @iliabylich I've not contributed to parser in a long time. So I'm not sure if it was correct to tag you both for review. Sorry If I caused noise. Pls tell me if I overstep with that review request. |
| Parser::Builders::Default.emit_procarg0 = true | ||
| Parser::Builders::Default.emit_encoding = true | ||
| Parser::Builders::Default.emit_index = true | ||
| Parser::Builders::Default.emit_lambda = true |
There was a problem hiding this comment.
Might be a good idea to add some comments explaining what those options do.
There was a problem hiding this comment.
There is documentation right above explaining what these options do.
There was a problem hiding this comment.
The documentation in the backwards compatibility section is extremely generic and doesn't mention any of the options specifically. I'm pretty sure most newcomers would have try some trouble trying to figure out what some of the more weird options mean.
There was a problem hiding this comment.
The fact that we even have these options is a consequence of our errors in judgement earlier. They should never be turned off explicitly, and in particular newcomers shouldn't set them to any other value. If someone really wants to know what they do, reading the source is fine.
There was a problem hiding this comment.
Fair enough. :-) This can also be made clearer in the docs, but I guess it's not a big deal.
There was a problem hiding this comment.
They should never be turned off explicitly, and in particular newcomers shouldn't set them to any other value.
That is the reason I propose to expose a (or the) #modernize method and document it. It removes even needing to know the details of what modernize means.
There was a problem hiding this comment.
That is the reason I propose to expose a (or the)
#modernizemethod and document it.
That's impossible. Ever changing what that method does would break backwards compatibility, thus defeating the purpose of having it in first place.
There was a problem hiding this comment.
That's impossible. Ever changing what that method does would break backwards compatibility, thus defeating the purpose of having it in first place.
True. The method itself would have to be versioned, defeating the propose.
For my use cases I'll probably detect "malformed from not modernized" ASTs and reject them early.
Okay, so can we merge the PR as is? I think its an incremental improvement over the current state.
|
Thanks for review / merge. |
emit_arg_inside_procarg0. This allows users to mirror what
Parser::Builders::Default.modernizedoes.