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

excluding vendor by default #806

Merged
merged 1 commit into from
Feb 14, 2014

Conversation

jeremyolliver
Copy link
Contributor

I'm not sure what the intentions are on providing defaults, I note that there's currently no files excluded from the default configuration. I'm proposing that the rubocop defaults, should exclude files matching vendor/bundle/**. The reason for this being that it's a default bundle install location for CI servers (it's the default path bundler uses for the --deployment flag, and travis-ci installs the bundle there by default, and that running rubocop over third party dependencies, is not intended at all, as their style preferences are their own, and in any case, not what the user is seeking to check.

I can't think of any really good reasons why vendor/bundle would be wanted to be checked, and in any case, seems a logical default setting to me, unless I'm missing something. Anybody care to provide some counter arguments for why not, or does it sound like a good idea?

@bjeanes
Copy link

bjeanes commented Feb 13, 2014

I'd go one further and say all of vendor/ should be excluded. That directory is patently intended for code that you didn't write but are not installable by the language dependency manager.

@bbatsov
Copy link
Collaborator

bbatsov commented Feb 13, 2014

I guess this makes sense (ignoring the entire vendor/). @jonas054 @yujinakayama What do you think?

@yujinakayama
Copy link
Collaborator

👍 for vendor/.

@jonas054
Copy link
Collaborator

Some projects have vendor directories further down. How about **/vendor/**?

@bjeanes
Copy link

bjeanes commented Feb 13, 2014

That seems like an increased risk of false positives. I have directories
called vendor in an api that deals with integrations with vendors.
On 13 Feb 2014 09:47, "Jonas Arvidsson" notifications@github.com wrote:

Some projects have vendor directories further down. How about /vendor/
?


Reply to this email directly or view it on GitHubhttps://github.com//issues/806#issuecomment-35005071
.

@jeremyolliver
Copy link
Contributor Author

I'm inclined to agree with @bjeanes - false positives probably aren't worth the risk given most projects won't have that setup.

@jeremyolliver
Copy link
Contributor Author

I Just added a commit here to implement exclusion of vendor/** as that looks like where consensus is heading. Feel free to continue discussion or merge when you're happy

@bbatsov
Copy link
Collaborator

bbatsov commented Feb 14, 2014

@jeremyolliver Please, update the changelog as well.

@bjeanes
Copy link

bjeanes commented Feb 14, 2014

@jeremyolliver also double check your commit message. It looks like you accidentally committed Please enter the commit message for your changes. Lines starting

@jeremyolliver
Copy link
Contributor Author

Whoops. done and done. I rebased, added an entry into the changelog with a similar format and corrected the extra line in the commit message

We shouldn't be checking third party code which we don't control for style.
Common items in here may be a bundle install to vendor/bundle or other packaged
third party items.
bbatsov added a commit that referenced this pull request Feb 14, 2014
@bbatsov bbatsov merged commit fa10d0d into rubocop:master Feb 14, 2014
@bbatsov
Copy link
Collaborator

bbatsov commented Feb 14, 2014

👍

@jeremyolliver
Copy link
Contributor Author

Thanks :)

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

Successfully merging this pull request may close these issues.

5 participants