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 a fix for Symbol#inspect on 3.0, 3.1 #361

Merged
merged 6 commits into from
Feb 1, 2024

Conversation

prikha
Copy link
Contributor

@prikha prikha commented Jan 18, 2024

@prikha prikha marked this pull request as ready for review January 18, 2024 15:41
@prikha prikha force-pushed the custom-symbol-node-serialization branch from 6b83aaf to 4a0ac3e Compare January 18, 2024 16:23
@prikha prikha force-pushed the custom-symbol-node-serialization branch from 63421ca to c90147d Compare January 18, 2024 16:25

# mutant:disable
def dispatch
if RUBY_VERSION < '3.2' && value[-1] == '='
Copy link
Owner

Choose a reason for hiding this comment

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

@prikha if you had mutation tested this on 3.1, it should have forced you into value[-1].eql?('=') which is better code as it uses the non coercing #== (even if coercion does not trigger here in the first place, its just less powerful primitive).

But in addition, I do not think we should pull a string out to than compare with a literal. We can AFAIK just directly uses regexps so value.match?(/=\z/). This creates no intermediary objects and avoids also to access out of bounds on empty symbols.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied

@prikha prikha requested a review from mbj January 18, 2024 19:13

# mutant:disable
def dispatch
if RUBY_VERSION < '3.2' && value.match?(/=\z/)
Copy link
Owner

Choose a reason for hiding this comment

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

@prikha I'm well aware I probably did this wrong within unparser at other places, but I think we should use: RUBY_VERSION < '3.2.' note the extra . If ruby ever where to get a minor version of like: 3.11 it would despite logically being bigger, be lexicographical sorted wrong.

Copy link
Owner

Choose a reason for hiding this comment

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

Next question, how do we know that only = suffix symbols are affected?

I wounder if we should instead do something "stupid" and on RUBY < '3.2.' do a round trip on #inspect and if that fails add the quotes, and raise if that also fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's go with a single roundtrip. Second one seems to be an overkill to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied

@prikha prikha force-pushed the custom-symbol-node-serialization branch 2 times, most recently from e3ae40e to d421720 Compare January 23, 2024 11:32
@prikha prikha force-pushed the custom-symbol-node-serialization branch from d421720 to 03c5722 Compare January 23, 2024 11:57
@prikha prikha requested a review from mbj January 24, 2024 09:26
@mbj mbj merged commit 13419d8 into mbj:main Feb 1, 2024
21 checks passed
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.

May break on Ruby versions below 3.2.0 due to non-evaluatable Symbol#inspect
2 participants