Skip to content

Fix charlist formatting issue on '\"' #13364

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

Merged
merged 4 commits into from
Feb 23, 2024

Conversation

sabiwara
Copy link
Contributor

Close #13363

@josevalim
Copy link
Member

I am worried this may introduce other regressions. What if we check if the string has a “ and, if so, we use the sigil c with single quotes?

@sabiwara
Copy link
Contributor Author

I am worried this may introduce other regressions.

This is a fair point, I actually started working on a prop-based tests to catch some other cases and indeed it did.

What if we check if the string has a “ and, if so, we use the sigil c with single quotes?

Could be a strategy, but it might be a bit more tedious to check for interpolation (not a big deal).
I was also evaluating replacing the whole String.replace pipe with a recursive function, will see how it goes and report back.

@sabiwara
Copy link
Contributor Author

@josevalim a91b626 seems to pass everything my prop-based test throws at it, and might be safer than the replace approach, WDYT?

@josevalim
Copy link
Member

I am honestly a bit worried because we are adding logic for handling '' => ~c"", which is a change of delimiters, into general delimiter logic.

So I would still prefer to emit ~c'' if there are ". It has an added benefit of not really changing the contents of the string.

@sabiwara
Copy link
Contributor Author

I am honestly a bit worried because we are adding logic for handling '' => ~c"", which is a change of delimiters, into general delimiter logic.

That's a great point. Will go with your approach!

@sabiwara
Copy link
Contributor Author

@josevalim we should be good now, and the property-based tests still pass ✅

Copy link
Member

@josevalim josevalim left a comment

Choose a reason for hiding this comment

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

Beautiful, please backport to v1.16 as well!

@sabiwara sabiwara merged commit 5ec63f0 into elixir-lang:main Feb 23, 2024
@sabiwara sabiwara deleted the format-charlist-escape branch February 23, 2024 15:02
sabiwara added a commit to sabiwara/elixir that referenced this pull request Feb 23, 2024
@sabiwara
Copy link
Contributor Author

Backported 677ad61
FYI had to fix a small test that had been removed on main a99a9cf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

formatter incorrectly transitions single quote charlists to sigil_c
2 participants