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

Make Memoist.escape_punctuation compatible with MRI 2.7 #82

Merged
merged 1 commit into from
Nov 4, 2019

Conversation

casperisfine
Copy link

In https://bugs.ruby-lang.org/issues/16150 MRI merge an experimental feature that makes Symbol#to_s return a frozen string.

So unless that experiment is reverted before release, escape_punctuation need to handle string.to_s being potentially frozen.

@matthewrudy

@casperisfine
Copy link
Author

The CI failures seem to be due to outdated travis config. Other than that it's green.

@yahonda
Copy link

yahonda commented Oct 3, 2019

Hi, I'd like this pull request reviewed because this change should address one of Rails CI failure reported at https://buildkite.com/rails/rails/builds/64100#92ed9e75-c005-42a2-af7d-64b2997f0436/984-1452

@sebjacobs
Copy link
Collaborator

I'm sorry for the delay but my brother, @matthewrudy, recently passed away.

According to recent activity on the original ruby issue 1: it looks like
there might be plans to revert the experiment for the time being, however I
am happy to help with getting this change merged regardless.


# A String can't end in both ? and !
if string.sub!(/\?\Z/, '_query'.freeze)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if it is worth retaining these explicit .freeze calls for older versions of ruby (2.2 and below). If not then it would be good to add something to the commit message which explains the reasoning behind dropping them.

Copy link
Author

Choose a reason for hiding this comment

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

No you are right, I'll add them back.

@casperisfine
Copy link
Author

I'm sorry for the delay but my brother, @matthewrudy, recently passed away.

I'm sorry for your loss.

@casperisfine
Copy link
Author

As for CI failures @unasuke opened #84.

@sebjacobs
Copy link
Collaborator

Thanks @casperisfine. This looks good.

@sebjacobs sebjacobs merged commit ee0c80a into matthewrudy:master Nov 4, 2019
@casperisfine
Copy link
Author

Thanks for the merge.

Is there anything you'd like included in the next release that I can help with?

@ghiculescu
Copy link

For those who just saw this (like me), 0.16.1 and up include this fix.

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.

5 participants