[Fixes #44] Use parser's new emit_forward_arg by default #52
Conversation
| @@ -14,6 +14,8 @@ module AST | |||
| # parser = Parser::Ruby25.new(builder) | |||
| # root_node = parser.parse(buffer) | |||
| class Builder < Parser::Builders::Default | |||
| self.emit_forward_arg = true | |||
There was a problem hiding this comment.
Shouldn't we leave it to RuboCop to set this, for the sake of consistency in rubocop-ast?
There was a problem hiding this comment.
Contrary to the other emit_, this is really fixing an error in the AST. We are < 1.0 so I think it's best to set this default now.
| This gem is meant to be compatible with all settings. For example, to have `-> { ... }` emitted | ||
| This gem, by default, uses most [legacy AST output from parser](https://github.com/whitequark/parser/#usage), except for `emit_forward_arg` which is set to `true`. | ||
|
|
||
| The main `RuboCop` gem uses these defaults (and is currently only compatible with these), but this gem can be used separately from `RuboCop` and is meant to be compatible with all settings. For example, to have `-> { ... }` emitted |
There was a problem hiding this comment.
I think a more salient point here is that most tools using parser use the legacy format, that's why it's a safer default.
There was a problem hiding this comment.
I think a more salient point here is that most tools using parser use the legacy format
Quite possible (by choice or because they aren't aware of it). I know some are using the "modern" AST (unparser, ruby-next), but my tool (DeepCover) doesn't, by ignorance first and by choice now 😆
that's why it's a safer default.
I'm not sure what you mean by "safer".
My objective is to be useful. Have you actually looked closely at the emit_... flags?
In my opinion, while they reflect an actual difference in the language, all of them are insignificant over-complexifications for the vast majority of tools and Rubyists.
For example: -> {} and lambda {} are indeed different in theory because one could, for example, redefine the method lambda while the first form woudn't be affected. I'd venture to say that the vast majority of parser uses don't care one bit about that difference. For RuboCop there might be a single cop caring about it (fixing the preference of one form over the other); all other cops probably prefer the simplicity of having to deal with the same AST for both constructs.
Some are even more obscure. I personally much prefer an AST of (send _ :[]= ...); it's how I think about foo[42]= :bar. I've yet to see foo.[]=(42, :bar) in the wild and it's quite sad that it forced parser to introduce a different AST for this.
emit_forward is different. It was actually a bad decision and the AST emitted without it is of a strange structure (forward-args not inside an args, ...) and is not forward compatible with 2.8. My opinion is that it should be turned on by default by all tools that are aware of it.
Are there emit_... that you feel are actually useful when turned on?
There was a problem hiding this comment.
that's why it's a safer default.
I meant it's less likely to surprise someone with the modern parser output. Maybe that's not a big problem in practice.
My objective is to be useful. Have you actually looked closely at the emit_... flags?
Yeah, I have. Generally most of the changes didn't add much value for us, that's why I never felt compelled to update the code. The biggest practical issue is obviously the big number of extensions that would suffer if we update the format, so for such changes we should always have a good reason. I'm fine with updating forward-args, as it has a relatively small impact, but I want to make sure people understand our rationale.
marcandre
force-pushed
the
forward
branch
3 times, most recently
from
96b2d05
to
6dce0e6
Compare
Bump minimum RuboCop supported as we need the following commit: rubocop/rubocop@3aeadd921fb526
No description provided.