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

Add --exclude-limit to configure --auto-gen-config exclusion count #2004

Merged
merged 1 commit into from
Jul 11, 2015

Conversation

awwaiid
Copy link
Contributor

@awwaiid awwaiid commented Jul 1, 2015

I adapted @jonas054 work from #998 here.

@awwaiid awwaiid force-pushed the auto_gen_exclude_limit branch from fb31477 to d015769 Compare July 1, 2015 04:05
@awwaiid awwaiid force-pushed the auto_gen_exclude_limit branch 2 times, most recently from bf270db to 7d73bc5 Compare July 1, 2015 04:51
@@ -23,6 +23,12 @@ class << self
attr_accessor :config_to_allow_offenses
end

def file_started(_file, file_info)
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 don't like passing in the args here -- I wish that #started took the cli_options instead of #file_started. But I didn't want to change the API.

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 should note that the code I based this on just set an instance variable inside of the formatter from the cli lib directly, which I also didn't like much -- but I can totally put it back to that if you prefer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This way is good, and it's a public API, so you're right in not changing it.

@awwaiid awwaiid force-pushed the auto_gen_exclude_limit branch from 7d73bc5 to b6b6b83 Compare July 3, 2015 20:37
@bbatsov
Copy link
Collaborator

bbatsov commented Jul 4, 2015

@jonas054 Are you going to review this or should I?

@awwaiid awwaiid force-pushed the auto_gen_exclude_limit branch from b6b6b83 to f5d7ed3 Compare July 4, 2015 19:36
@awwaiid
Copy link
Contributor Author

awwaiid commented Jul 4, 2015

(I keep pushing minor conflict changes to make merging trivial :) )

@awwaiid awwaiid force-pushed the auto_gen_exclude_limit branch from f5d7ed3 to 98d6200 Compare July 4, 2015 20:44
file `.rubocop_todo.yml` contains configuration to disable cops that
currently detect an offense in the code by excluding the offending
files, or disabling the cop altogether once a file count limit has been
reached (the default is 15 files before the cop is completely disabled).
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's mentioned further down that the default is 15, so remove the text in parentheses.

@jonas054
Copy link
Collaborator

jonas054 commented Jul 6, 2015

Had some comments, but overall it looks good.

@bbatsov
Copy link
Collaborator

bbatsov commented Jul 9, 2015

@awwaiid Ping :-)

@awwaiid awwaiid force-pushed the auto_gen_exclude_limit branch from 98d6200 to c649865 Compare July 10, 2015 11:13
@awwaiid
Copy link
Contributor Author

awwaiid commented Jul 10, 2015

OK -- I applied all changes, and mainly ran into some difficulty making the constant in Options; first because I needed to refer to it from OptionsHelp, so I had to move OptionsHelp below options. Second because then a full module-path to the constant was too long to fit as-is in the help strings and still be a short line, so I made a short-name for it. I addressed the other feedback without issue.

Open to suggestions :)

@jonas054
Copy link
Collaborator

👍 Looks good. I can't think of a better solution for the problems you mention.

bbatsov added a commit that referenced this pull request Jul 11, 2015
Add --exclude-limit to configure --auto-gen-config exclusion count
@bbatsov bbatsov merged commit a1704ca into rubocop:master Jul 11, 2015
@bbatsov
Copy link
Collaborator

bbatsov commented Jul 11, 2015

👍 Looks good to me as well! Thanks!

@awwaiid
Copy link
Contributor Author

awwaiid commented Jul 11, 2015

YAY! :)

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.

3 participants