Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Fixes #44] Use parser's new emit_forward_arg by default #52

Merged
merged 1 commit into from Aug 1, 2020
Merged

[Fixes #44] Use parser's new emit_forward_arg by default #52

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions .github/workflows/rubocop.yml
View file
Open in desktop
Expand Up @@ -25,9 +25,9 @@ jobs:
title: [ null ]
include:
- { os: windows, rubocop: master, ruby: mingw }
- { rubocop: '0.84.0', ruby: 2.4, os: ubuntu }
- { rubocop: '0.84.0', ruby: head, os: ubuntu }
- { rubocop: '0.84.0', ruby: 2.4, os: ubuntu, coverage: true, title: 'Coverage' }
- { rubocop: '0.87.0', ruby: 2.4, os: ubuntu }
- { rubocop: '0.87.0', ruby: head, os: ubuntu }
- { rubocop: '0.87.0', ruby: 2.4, os: ubuntu, coverage: true, title: 'Coverage' }
- { rubocop: master, ruby: 2.7, os: ubuntu, modern: true, title: 'Specs "modern"' }
- { rubocop: master, ruby: 2.7, os: ubuntu, internal_investigation: true, modern: true, title: 'Coding Style' }

Expand Down Expand Up @@ -87,7 +87,7 @@ jobs:
matrix:
os: [ ubuntu ]
ruby: [ 2.4, 2.7 ]
rubocop: [ '0.84.0', master ]
rubocop: [ '0.87.0', master ]

steps:
- name: checkout
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Expand Up @@ -17,6 +17,7 @@

### Changes

* [#44](https://github.com/rubocop-hq/rubocop-ast/issue/44): **(Breaking)** Use `parser` flag `self.emit_forward_arg = true` by default. ([@marcandre][])
* [#86](https://github.com/rubocop-hq/rubocop-ast/pull/86): `PairNode#delimiter` and `inverse_delimiter` now accept their argument as a named argument. ([@marcandre][])

## 0.2.0 (2020-07-19)
Expand Down
5 changes: 3 additions & 2 deletions README.md
View file
Open in desktop
Expand Up @@ -35,8 +35,9 @@ See the [docs site](https://docs.rubocop.org/rubocop-ast) for more details.

### Parser compatibility switches

The main `RuboCop` gem uses [legacy AST output from parser](https://github.com/whitequark/parser/#usage).
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

as `LambdaNode` instead of `SendNode`:

```ruby
Expand Down
2 changes: 2 additions & 0 deletions lib/rubocop/ast/builder.rb
View file
Open in desktop
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we leave it to RuboCop to set this, for the sake of consistency in rubocop-ast?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough.


NODE_MAP = {
and: AndNode,
alias: AliasNode,
Expand Down
2 changes: 1 addition & 1 deletion rubocop-ast.gemspec
View file
Open in desktop
Expand Up @@ -29,7 +29,7 @@ Gem::Specification.new do |s|
'bug_tracker_uri' => 'https://github.com/rubocop-hq/rubocop-ast/issues'
}

s.add_runtime_dependency('parser', '>= 2.7.0.1')
s.add_runtime_dependency('parser', '>= 2.7.1.4')

s.add_development_dependency('bundler', '>= 1.15.0', '< 3.0')

Expand Down
5 changes: 4 additions & 1 deletion spec/spec_helper.rb
View file
Open in desktop
Expand Up @@ -8,7 +8,10 @@
end

require 'rubocop-ast'
RuboCop::AST::Builder.modernize if ENV['MODERNIZE']
if ENV['MODERNIZE']
RuboCop::AST::Builder.modernize
RuboCop::AST::Builder.emit_forward_arg = false # inverse of default
end

RSpec.shared_context 'ruby 2.3', :ruby23 do
let(:ruby_version) { 2.3 }
Expand Down