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

changed str << to str += #255

Merged
merged 1 commit into from
May 11, 2021
Merged

changed str << to str += #255

merged 1 commit into from
May 11, 2021

Conversation

MadBomber
Copy link
Contributor

rake test before change results in about 5 errors.

rake test after change had no errors.

@leejarvis
Copy link
Owner

What version of Ruby are you using, @MadBomber? We have specs that run against all of these versions and they're currently all passing, so if there's a version the tests are failing on I'd want to add that to the matrix.

@leejarvis
Copy link
Owner

Ah I've just read the issue. You must be running with frozen string literals. I'm up for fixing this but I'm still interested in how/why you received this error message. Do you have # frozen_string_literal: true at the top of your Ruby files or something else?

@MadBomber
Copy link
Contributor Author

I set the system environment variable RUBYOPT to be:

export RUBYOPT=-W0 --enable-frozen-string-literal

So I could start catching problems that could account for some weird side-effects that I was seeing in some production code. Then my common CLI utilities stopped working.

I use your gem in my cli_helper gem which I use to build all of my cli utilities. I tracked it back to the use of << instead of += for string concat.

@olleolleolle
Copy link
Collaborator

I have one change request: the chmod of the file changed to 755, perhaps that can be changed back?

@leejarvis
Copy link
Owner

I think we should fix this but we should also enable frozen strings in Slop using the # frozen_string_literal: true pragma

@MadBomber
Copy link
Contributor Author

I have one change request: the chmod of the file changed to 755, perhaps that can be changed back?

yes.

@leejarvis
Copy link
Owner

Merged, thanks! -- not enabling frozen_string_literal just yet but this is now on master

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