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 a Strict mode to Style/MutableConstants #6126

Merged
merged 1 commit into from Feb 16, 2019
Merged

Add a Strict mode to Style/MutableConstants #6126

merged 1 commit into from Feb 16, 2019

Conversation

rrosenblum
Copy link
Contributor

This has been something that I have wanted to do for a while. Right now, we only freeze literals that are assigned to constants. This leaves many constants unfrozen and therefore potentially unsafe.

I consider this to be experimental functionality. I'm not sure how many other people would be interested in this feature. I think I have all of the main use cases covered. I am trying to account for any edge cases now.

Something interesting that I learned from this is that concatenating frozen strings produces an unfrozen string.

@rrosenblum rrosenblum force-pushed the strict_mutable_constants branch 3 times, most recently from 2221cac to c73d470 Compare July 24, 2018 14:34
@rrosenblum rrosenblum force-pushed the strict_mutable_constants branch 7 times, most recently from bf73414 to 38e0ca6 Compare August 2, 2018 17:25
@rrosenblum rrosenblum changed the title [WIP] Add a Strict mode to Style/MutableConstants Add a Strict mode to Style/MutableConstants Aug 6, 2018
@rrosenblum
Copy link
Contributor Author

@bbatsov this is ready for review.

bbatsov
bbatsov reviewed Sep 8, 2018
View reviewed changes
.rubocop.yml Outdated Show resolved Hide resolved
@bbatsov
Copy link
Collaborator

bbatsov commented Sep 8, 2018

@rrosenblum Sorry about the slow-turnaround here. Overall it seems like a good change to me, but I've added a few inline comments.

@rrosenblum
Copy link
Contributor Author

Thanks for the feedback. I'll try to setup some rules for the comments that you made.

@rrosenblum rrosenblum force-pushed the strict_mutable_constants branch 6 times, most recently from 42cac6d to 89c8f93 Compare September 17, 2018 14:56
@rrosenblum rrosenblum mentioned this pull request Sep 19, 2018
Closed
@rrosenblum rrosenblum force-pushed the strict_mutable_constants branch 4 times, most recently from 94cb6ae to cb4f184 Compare September 28, 2018 13:53
@rrosenblum rrosenblum force-pushed the strict_mutable_constants branch 2 times, most recently from 99b40b5 to abdd447 Compare January 4, 2019 20:39
@Drenmi
Copy link
Collaborator

Drenmi commented Jan 22, 2019

@rrosenblum Looking good. If we can fix the false positive for heredocs, I think this is good enough to merge as an experimental feature. 🙂

koic
koic reviewed Jan 22, 2019
View reviewed changes
.rubocop.yml Outdated Show resolved Hide resolved
@rrosenblum rrosenblum force-pushed the strict_mutable_constants branch 2 times, most recently from 59dbdb3 to 6febf1a Compare February 8, 2019 14:33
@rrosenblum
Copy link
Contributor Author

I removed strict mode as the default setting for RuboCop, and reverted the changes made by that. This is ready for review again.

Drenmi
Drenmi approved these changes Feb 9, 2019
View reviewed changes
Copy link
Collaborator

@Drenmi Drenmi left a comment

Choose a reason for hiding this comment

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

I’m okay with this, with some small nits. Letting @koic make the final call. 🚀

config/default.yml Show resolved Hide resolved
lib/rubocop/cop/style/mutable_constant.rb
end

def requires_parentheses?(node)
node.irange_type? ||
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 there is a good case for adding Node#range_type?, but that could be considered out of scope for this change. 🙂

lib/rubocop/cop/style/mutable_constant.rb Outdated Show resolved Hide resolved
koic
koic reviewed Feb 13, 2019
View reviewed changes
spec/rubocop/cop/style/redundant_freeze_spec.rb Outdated
@@ -23,6 +23,11 @@
it_behaves_like 'immutable objects', '1.5'
it_behaves_like 'immutable objects', ':sym'
it_behaves_like 'immutable objects', ':""'
it_behaves_like 'immutable objects', "ENV['foo']"
it_behaves_like 'immutable objects', "'foo'.count"
Copy link
Member

Choose a reason for hiding this comment

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

String object does not have count method, does this mean String#size or String#length?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

String does support count. Without arguments it works like size and length. It can take a parameter of a pattern to count.

@koic
Copy link
Member

koic commented Feb 13, 2019

Several reviews by @Drenmi are left, but dc3631d in other points looks good to me 🌟
I have a question. Is 6febf1a different commit from dc3631d? If so, if you open 6febf1a as a different PR, dc3631d would like to merge first if the following has been updated.

@koic koic mentioned this pull request Feb 15, 2019
Merged
@rrosenblum
Copy link
Contributor Author

6febf1a is different from dc3631d. I can break these out into separate PRs.

@koic koic merged commit ec5673b into rubocop:master Feb 16, 2019
@koic
Copy link
Member

koic commented Feb 16, 2019

This looks good! Thanks @rrosenblum.

@koic koic mentioned this pull request Feb 17, 2019
Merged
8 tasks
koic added a commit to koic/rubocop that referenced this pull request Feb 17, 2019
@koic
Add range_type? which means irange_type? and erange_type?
4105b22 
Follow up of rubocop#6126 (comment).

This PR adds `range_type?` method which means both
`irange_type?` method and `erange_type?` method.
@Drenmi Drenmi mentioned this pull request Feb 19, 2019
Merged
@rrosenblum rrosenblum mentioned this pull request Feb 20, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@koic koic koic left review comments

@bbatsov bbatsov bbatsov left review comments

@Drenmi Drenmi Drenmi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@rrosenblum @bbatsov @Drenmi @koic