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.

Sign up for GitHub

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

Jump to bottom

Add NumericLiteralPrefix cop #3177

Merged
merged 2 commits into from
Jun 18, 2016
Merged

Add NumericLiteralPrefix cop #3177

merged 2 commits into from
Jun 18, 2016

Conversation

tejasbubane
Copy link
Contributor

@tejasbubane tejasbubane commented May 28, 2016 •
edited
Loading

Closes #3123

This cop checks for octal numbers starting with 0 and 0O and
corrects them to 0o.

Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • 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.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • 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
Copy link
Collaborator

bbatsov commented May 28, 2016

P.S. This should also be a rule in the style guide.

jonas054
jonas054 reviewed May 29, 2016
View reviewed changes
lib/rubocop/cop/style/octal_literals.rb
# encoding: utf-8
# frozen_string_literal: true

require 'pry'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remember to remove this line.

@tejasbubane tejasbubane mentioned this pull request Jun 2, 2016
Merged
@tejasbubane tejasbubane changed the title WIP: Add octal literals cop Add octal literals cop Jun 2, 2016
@tejasbubane
Copy link
Contributor Author

@bbatsov Added a rule to the styleguide and updated this PR

@bbatsov
Copy link
Collaborator

bbatsov commented Jun 2, 2016

Btw, ideally this cop should be configurable. While few people would prefer 0O, I know a lot of people are fond of just prefixing octals with 0.

@tejasbubane
Copy link
Contributor Author

@bbatsov Right. I will make this configurable.

Also regarding the hex numbers and similarly binary numbers, should we have separate cops for them or one cop for all three (octal, hex and binary)? Looks like functionality would be more or less the same.

@bbatsov
Copy link
Collaborator

bbatsov commented Jun 6, 2016

Also regarding the hex numbers and similarly binary numbers, should we have separate cops for them or one cop for all three (octal, hex and binary)? Looks like functionality would be more or less the same.

Guess one cop should be fine.

@bbatsov
Copy link
Collaborator

bbatsov commented Jun 10, 2016

@tejasbubane ping :-)

@tejasbubane tejasbubane changed the title Add octal literals cop Add NumericLiteralPrefix cop Jun 13, 2016
@tejasbubane
Copy link
Contributor Author

@bbatsov Sorry for the delay. Made the cop generic for all numeric literals (octal, hex, binary and decimal). Added configuration option for octal literals to use just 0 instead of 0o or 0O. The default will be 0o.

Corresponding PR for style-guide is here: rubocop/ruby-style-guide#579

Decimal prefixes came up in the discussion in above style-guide PR. I have made the cop to discourage the use of decimal prefixes 0d or 0D since they seem to be very less known and not elegant.

jonas054
jonas054 reviewed Jun 14, 2016
View reviewed changes
spec/rubocop/cop/style/numeric_literal_prefix_spec.rb
expect(cop.messages).to be_empty
end

it 'autocorrects a octal literal starting with 0O or 0o' do
Copy link
Collaborator

Choose a reason for hiding this comment

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

an octal literal

@jonas054
Copy link
Collaborator

Looks good! I had some small comments.

@tejasbubane
Add literal prefixes cop
8842ba0 
Closes rubocop#3123

This cop checks for numeric literals using uppercase prefixes and
corrects them to use lowercase of no prefix (in case of decimals).
@tejasbubane
Make Style/NumericLiteralPrefix configurable for octal literals
9ffa30d 
Can use `EnforcedOctalStyle` => `zero_only` to use only `0` for
octal literals instead of `0o` or `0O`.
@tejasbubane
Copy link
Contributor Author

Thanks @jonas054 👍 Those were some really good comments. I have fixed them.

@tejasbubane
Copy link
Contributor Author

I have kept two commits separate - one for creating the cop, and other for making it configurable. Let me know if you want them squashed.

@jonas054
Copy link
Collaborator

👍 LGTM

@bbatsov bbatsov merged commit 4557b63 into rubocop:master Jun 18, 2016
@tejasbubane tejasbubane deleted the add-octal-literals-cop branch June 19, 2016 05:07
Neodelf pushed a commit to Neodelf/rubocop that referenced this pull request Oct 15, 2016
@tejasbubane @Neodelf
Add NumericLiteralPrefix cop (rubocop#3177)
1ad259b 
Closes rubocop#3123

This cop checks for numeric literals using uppercase prefixes and
corrects them to use lowercase of no prefix (in case of decimals).

Can use `EnforcedOctalStyle` => `zero_only` to use only `0` for
octal literals instead of `0o` or `0O`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tejasbubane @bbatsov @jonas054