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 RuboCop linting and use frozen_string_literal everywhere #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ur5us
Copy link
Contributor

@ur5us ur5us commented Feb 10, 2020

No description provided.

@ur5us ur5us requested a review from heaven February 10, 2020 01:33
@ur5us
Copy link
Contributor Author

ur5us commented Feb 17, 2020

@heaven Please let me know if this PR is cramming too many concerns into one issue, especially the RuboCop formatting related ones. I could potentially separate those from the frozen_string_literal changes.

Also, are you planning on cutting a new release by any chance?

@heaven
Copy link
Owner

heaven commented Feb 17, 2020

Hi @ur5us, I'm a bit concerned about the frozen string literals. As you can see there are 0 tests and I'm not sure what this change can affect. As far as I know, Matz decided not to enable this feature by default in ruby 3.0 and for a good reason. I'd better accept a solution to enable this globally, e.g. via ENV variables than using these magic comments, so that anyone could decide whether they need this feature enabled in their projects or not.

@ur5us
Copy link
Contributor Author

ur5us commented Feb 17, 2020

@heaven Curious, do you have a link? My understanding is that string will be frozen/immutable by default and you’ll need to opt out by adding # frozen_string_literal: false at the top.

The reason I decided to add # frozen_string_literal: true everywhere is because there were a lot of #freeze calls scattered around and only 2 places need a mutable string which you get by using String.new, s. https://www.mikeperham.com/2018/02/28/ruby-optimization-with-one-magic-comment/#notes. But yeah, not having tests is a concern.

@ur5us ur5us force-pushed the feature/linting-and-freeze branch from 8d0e363 to 944890c Compare February 17, 2020 21:15
@heaven
Copy link
Owner

heaven commented Feb 18, 2020

I saw this discussion some time ago https://bugs.ruby-lang.org/issues/11473#note-53

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.

2 participants