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.

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

Regression on Exclude #757

Closed
agrimm opened this issue Jan 23, 2014 · 6 comments
Closed

Regression on Exclude #757

agrimm opened this issue Jan 23, 2014 · 6 comments
Labels

Comments

@agrimm
Copy link
Contributor

agrimm commented Jan 23, 2014

Exclude has regressed between 0.16.0 and 0.17.0

If I have the following bad code in lib/bad.rb:

# lib/bad.rb: Flip flop
DATA.each_line do |line|
  print line if (line =~ /begin/)..(line =~ /end/)
end

And have the following .rubocop.yml configuration:

FlipFlop:
  Exclude: [lib/bad.rb]

The Exclude is honoured in 0.16.0, but not in 0.17.0. This regression happens with other cops as well. (FlipFlop was introduced in 0.16.0, so it's not that the cop didn't exist in that version)

@jonas054
Copy link
Collaborator

According to git bisect,

d3dbc37330dba4173af7d250fb7f13ebdb7420e4 is the first bad commit
commit d3dbc37330dba4173af7d250fb7f13ebdb7420e4
Author: Yuji Nakayama <...>
Date:   Tue Jan 7 19:58:06 2014 +0900

    Fix a bug some offences were discarded with cop specific target paths

@yujinakayama Can you take a look?

@yujinakayama
Copy link
Collaborator

@jonas054 Sure.

@yujinakayama
Copy link
Collaborator

In my environment, git bisect reports:

df650f7241ea625af959433c049334edbe7f796a is the first bad commit
commit df650f7241ea625af959433c049334edbe7f796a
Author: Bozhidar Batsov <...>
Date:   Tue Jan 14 22:39:58 2014 +0200

    Support globbing in cop exclude/include

I guess the cause of the bug is in handling of absolute/relative paths. I'll fix this.

@bbatsov By the way, I don't understand that File.basename is used in the path matching logic. What was this intended for?

@bbatsov
Copy link
Collaborator

bbatsov commented Jan 24, 2014

Yes it was intentional, so I wouldn't exactly call this a regression. Before the change I made, strings were implicitly converted to regexps, now they are treated as globs. Relative paths were problematic to handle without keeping track of the config file from which the exclude/include originated, so I took a shortcut (matching only against the basename). Seems it wasn't a good idea, so feel free to fix it.

@agrimm, until a better solution is present, this should work:

FlipFlop:
  Exclude: 
    - !ruby/regexp /lib\/bad.rb/

@tamird
Copy link
Contributor

tamird commented Jan 24, 2014

That example doesn't work -- I keep getting TypeError: can't convert Fixnum into String trying to read YAML that looks like that.

Every forward slash has to be escaped, and the entire regex needs to be wrapped in forward slashes. :(

@bbatsov
Copy link
Collaborator

bbatsov commented Jan 24, 2014

@tamird Indeed. I got slightly carried away.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants