[Fix #3511] Style/TernaryParentheses false positive #3513

dreyks · 2016-09-19T13:43:32Z

Fix false positive for a ternary with ranges

Fixes #3511

Wrote [good commit messages][1].
Commit message starts with [Fix #issue-number] (if the related issue exists).
Used the same coding conventions as the rest of the project.
Feature branch is up-to-date with master (if not - rebase it).
Squashed related commits together.
Added tests.
All tests are passing.
The new code doesn't generate RuboCop offenses.
The PR relates to only one subject with a clear title
and description in grammatically correct, complete sentences.

bbatsov · 2016-09-19T13:50:11Z

The fix looks good, but it also needs a changelog entry.

dreyks · 2016-09-19T13:56:09Z

done

bbatsov · 2016-09-19T14:05:27Z

CHANGELOG.md

@@ -1,6 +1,7 @@
 # Change log

 ## master (unreleased)
+[#3513](https://github.com/bbatsov/rubocop/pull/3513): Fix false positive in `Style/TernaryParentheses` for a ternary with ranges. ([@dreyks][])


See the changelog for older releases. There should be a Bugs fixed section here.

Yup, fixed this.

Drenmi · 2016-09-19T14:08:18Z

lib/rubocop/cop/style/ternary_parentheses.rb

@@ -91,7 +91,7 @@ def redundant_parentheses_enabled?
        end

        def parenthesized?(node)
-          node.source =~ /^\(.*\)$/
+          node.type == :begin


There's a predicate method on Node for this:

node.begin_type?

Also, I'm not entirely sure why this works. 😅 With the changed implementation, does the method name still make sense?

I'm new to this whole AST thing but empirically I realized that a braced expression has a :begin root node, while non-braced has anything but it... I will change to begin_type?

And all the previous tests pass, so it seems that I didn't break anything >.<

Okay. Makes sense to me. Nice job fixing this. 😀

dreyks · 2016-09-19T14:56:56Z

💚

bbatsov · 2016-09-19T15:05:05Z

I'm new to this whole AST thing but empirically I realized that a braced expression has a :begin root node, while non-braced has anything but it... I will change to begin_type?

And all the previous tests pass, so it seems that I didn't break anything >.<

There's one small caveat. Something like x; y will also introduce a begin node, so ideally you should still check if this begin node starts with a ( and perhaps add a test for the x; y scenario. (begin nodes simply indicate there are more than 1 expression in a scope).

dreyks · 2016-09-19T15:11:10Z

I've just checked x; y generates a :send root node

dreyks · 2016-09-19T15:13:04Z

I've added a couple of tests. What else should I check for?

bbatsov · 2016-09-19T17:47:39Z

I've just checked x; y generates a :send root node

[2] pry(main)> Parser::CurrentRuby.parse("x; y")
=> s(:begin,
  s(:send, nil, :x),
  s(:send, nil, :y))

Doesn't look like a send root node to me.

bbatsov · 2016-09-19T17:49:25Z

spec/rubocop/cop/style/ternary_parentheses_spec.rb

+
+    context 'with several expressions on one line' do
+      it_behaves_like 'code without offense',
+                      'foo; bar ? a: b'


bbatsov · 2016-09-19T17:50:07Z

Anyways, I recalled that in the context of a ? this won't matter as ? binds stronger than ;.

bbatsov · 2016-09-19T17:50:35Z

spec/rubocop/cop/style/ternary_parentheses_spec.rb

+                      '(foo..bar).include?(baz) ? a : b'
+    end
+
+    context 'with several expressions on one line' do


Guess I misled you and we can drop this example.

dreyks · 2016-09-19T18:35:15Z

💚

bbatsov · 2016-09-20T07:42:06Z

👍

reitermarkus · 2016-09-21T13:11:17Z

Great to see this fixed, just ran into this with this nice line:

        "cellar" => (cellar = bottle_spec.cellar).is_a?(Symbol) ? cellar.inspect : cellar,

* bbatsov/master: (80 commits) [Fix rubocop#3540] Make `Style/GuardClause` register offense for instance & singleton methods [Fix rubocop#3436] issue related to Rails/SaveBang when returning non-bang call from the parent method Allow `#to_yml` to produce single-quoted strings Add support for StyleGuideBaseURL and update rules Add spec for the existing style guide URL implementation Fix the changelog Edited regular expression for normal case to fix issues rubocop#3514 and rubocop#3516 (rubocop#3524) Add a rake task for generation a new cop (rubocop#3533) [Fix rubocop#3510] Various bug fixes for SafeNavigation (rubocop#3517) [Fix rubocop#3512] Change error message of `Lint/UnneededSplatExpansion` for array (rubocop#3526) Fix false positive in `Lint/AssignmentInCondition` (rubocop#3520) (rubocop#3529) Rename a mismatched filename (rubocop#3523) Fix a broken changelog entry [Fix rubocop#3511] Style/TernaryParentheses false positive (rubocop#3513) Fix the release notes for 0.43 Cut 0.43.0 [Fix rubocop#3462] Don't flag single-line methods Fix false negatives in `Rails/NotNullColumn` cop (rubocop#3508) Remove unused doubled methods (rubocop#3509) [Fix rubocop#3485] Make OneLineConditional cop ignore empty else (rubocop#3487) ...

)

bbatsov reviewed Sep 19, 2016

View reviewed changes

Drenmi reviewed Sep 19, 2016

View reviewed changes

bbatsov reviewed Sep 19, 2016

View reviewed changes

[Fix #3511] Style/TernaryParentheses false positive

85d9a09

bbatsov merged commit 51ed214 into rubocop:master Sep 20, 2016

Neodelf pushed a commit to Neodelf/rubocop that referenced this pull request Oct 15, 2016

[Fix rubocop#3511] Style/TernaryParentheses false positive (rubocop#3513

dcba344

)

rrosenblum mentioned this pull request Nov 2, 2016

undefined method `begin' for #<Parser::Source::Map::Operator:0x007fbe95fd5678> #3684

Closed

dreyks deleted the fix_ternary branch October 25, 2018 11:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix #3511] Style/TernaryParentheses false positive #3513

[Fix #3511] Style/TernaryParentheses false positive #3513

dreyks commented Sep 19, 2016 •

edited

Loading

bbatsov commented Sep 19, 2016

dreyks commented Sep 19, 2016

bbatsov Sep 19, 2016

dreyks Sep 19, 2016

Drenmi Sep 19, 2016

dreyks Sep 19, 2016 •

edited

Loading

Drenmi Sep 19, 2016

dreyks commented Sep 19, 2016

bbatsov commented Sep 19, 2016

dreyks commented Sep 19, 2016 •

edited

Loading

dreyks commented Sep 19, 2016

bbatsov commented Sep 19, 2016

bbatsov Sep 19, 2016

bbatsov commented Sep 19, 2016

bbatsov Sep 19, 2016

dreyks commented Sep 19, 2016

bbatsov commented Sep 20, 2016

reitermarkus commented Sep 21, 2016

[Fix #3511] Style/TernaryParentheses false positive #3513

[Fix #3511] Style/TernaryParentheses false positive #3513

Conversation

dreyks commented Sep 19, 2016 • edited Loading

Fix false positive for a ternary with ranges

bbatsov commented Sep 19, 2016

dreyks commented Sep 19, 2016

bbatsov Sep 19, 2016

Choose a reason for hiding this comment

dreyks Sep 19, 2016

Choose a reason for hiding this comment

Drenmi Sep 19, 2016

Choose a reason for hiding this comment

dreyks Sep 19, 2016 • edited Loading

Choose a reason for hiding this comment

Drenmi Sep 19, 2016

Choose a reason for hiding this comment

dreyks commented Sep 19, 2016

bbatsov commented Sep 19, 2016

dreyks commented Sep 19, 2016 • edited Loading

dreyks commented Sep 19, 2016

bbatsov commented Sep 19, 2016

bbatsov Sep 19, 2016

Choose a reason for hiding this comment

bbatsov commented Sep 19, 2016

bbatsov Sep 19, 2016

Choose a reason for hiding this comment

dreyks commented Sep 19, 2016

bbatsov commented Sep 20, 2016

reitermarkus commented Sep 21, 2016

dreyks commented Sep 19, 2016 •

edited

Loading

dreyks Sep 19, 2016 •

edited

Loading

dreyks commented Sep 19, 2016 •

edited

Loading