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

parts.rb: Use a mutable string #70

Merged
merged 2 commits into from
Aug 12, 2021
Merged

Conversation

olleolleolle
Copy link
Contributor

@olleolleolle olleolleolle commented Aug 11, 2021

Description

Use an explicitly mutable string, to support RUBYOPT=--enable-frozen-string-literal

I ran into it when trying to get support for that in octokit.rb, and into github-changelog-generator.

https://github.com/octokit/octokit.rb/pull/1354/checks?check_run_id=3298433252

Types of Changes

  • Bug fix, perhaps maintenance

Testing

Would it be OK to add the RUBYOPT setting mentioned above to CI? Oh, we don't have a running CI, I logged #71.

Supports --enable-frozen-string-literal.
@Lewiscowles1986
Copy link
Contributor

Lewiscowles1986 commented Aug 11, 2021

I got curious https://ruby-doc.org/core-2.3.0/String.html#method-i-2B-40
Then I made a CI in PR.
Then I manually cherry-pick this commit (using copy paste 😁 )

I Think Ruby 2.0 should be gone, but maybe this is the feedback this needs?

https://github.com/Lewiscowles1986/multipart-post/runs/3305811062?check_suite_focus=true

This library in Travis, was supporting (5 months ago build); Some quite old versions of Ruby.

I saw someone make an interesting commit elsewhere which was to have Ruby 2.5 do something with frozen string literals. I Do not understand this in a robust way. Does this help?

@ioquatix
Copy link
Member

I think I slightly prefer String.new to make a mutable buffer. What do you think?

@ioquatix
Copy link
Member

ioquatix commented Aug 12, 2021

Anything older than Ruby 2.3 we definitely don't care about.

Anything older than Ruby 2.6 I'm okay with dropping if it's inconvenient.

@Lewiscowles1986
Copy link
Contributor

I got 2.5-3.0 working with some playing around on a less tired brain
https://github.com/Lewiscowles1986/multipart-post/tree/frozen-stringd

@Lewiscowles1986
Copy link
Contributor

@olleolleolle will very likely have improvements to my takes on their code.

  • 2.5 patch is one that mimics their work on octokit.
  • I learned about frozen strings and workarounds from them.
  • Reading their commits I saw the need for CI.

Now I have green and I understand this better, the only thing I'm a little upset about is that the library requires downstream consumers to "".dup (through whatever means); which is large surface-area for coordination.

Might there be patterns where we can receive the string, and only mutate within this library; so that read is safe. Otherwise it feels like read should be read!. Is the buf name convention enough to say "this is a mutable outvar buffer".
Perhaps I have totally misunderstood.

@ioquatix ioquatix merged commit 724cc8e into socketry:master Aug 12, 2021
@olleolleolle olleolleolle deleted the patch-1 branch August 12, 2021 07:20
@olleolleolle
Copy link
Contributor Author

@Lewiscowles1986 👋 Hi! The --enable-frozen-string-literal is a sort of "nod to the future" where Ruby strings may become immutable. https://freelancing-gods.com/2017/07/27/an-introduction-to-frozen-string-literals.html is a nice blog post where Pat, an activist for "frozen string literals" to happen, describes the feature a little.

Nice to see you again, and I hope you had some fun!

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