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 '--failed-level' option #896

Merged
merged 1 commit into from
Mar 20, 2014
Merged

Conversation

hiroponz
Copy link

This feature is useful to display warning but to make successful build on CI server.

@@ -30,6 +30,10 @@ def parse(args)
require f
end

opts.on('--failed-level N', /[1-5]/, 'Minimum failed level.') do |n|
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think users know what the numbers 1-5 mean. We will have to explain it or let them use initial letters R, C, W, E, F (R for Refactor is not used anywhere).

Also, the explanation could be more clear. Perhaps "Minimum severity for exit with error code." would do.

@bbatsov
Copy link
Collaborator

bbatsov commented Mar 19, 2014

The code looks good to me. I have just a few remarks:

  • the option should be named fail-level instead of failed-level
  • it should be mentioned in the README and the CHANGELOG
  • the branch has to be rebased on top of the current master

@jonas054 @yujinakayama Anything else you'd like to add?

# @return [Symbol]
# severity.
# any of `:refactor`, `:convention`, `:warning`, `:error` or `:fatal`.
attr_reader :key
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the attribute names key and code are a bit ambiguous. I'd suggest name as a replacement for the current key.

@yujinakayama
Copy link
Collaborator

the option should be named fail-level instead of failed-level

👍

Also the commits need to be squashed.

@hiroponz
Copy link
Author

Fixed.

bbatsov added a commit that referenced this pull request Mar 20, 2014
@bbatsov bbatsov merged commit 5301b38 into rubocop:master Mar 20, 2014
@bbatsov
Copy link
Collaborator

bbatsov commented Mar 20, 2014

👍

@hiroponz
Copy link
Author

😄

@hiroponz hiroponz deleted the failed-level branch March 20, 2014 09:07
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.

4 participants