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 support for escape() vim function #1038

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

jphalip
Copy link
Contributor

@jphalip jphalip commented Nov 14, 2024

No description provided.

// Test consecutive characters that need escaping
typeText(commandToKeys("""echo escape('$$$$', '$')"""))
assertExOutput("""\$\$\$\$""")
}
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to extract this to a separate test class, perhaps under a builtin folder, so we can have separate files for each function, and allow splitting the test into more logical tests/asserts.

Copy link
Member

Choose a reason for hiding this comment

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

Probably worth adding a couple of tests for invalid method calls - too many parameters, not enough parameters, what happens with invalid data types, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@citizenmatt I've added a couple of tests with errors.

I also agree that it'd be nice to separate these buitlin functions out to separate class, but maybe this should be done in bulk in a separate PR?

@AlexPl292
Copy link
Member

Thank you for your PR! Can you please take a look and write a test on how this would work with multicharacter symbols like 😀?

@AlexPl292 AlexPl292 self-requested a review November 22, 2024 14:30
Copy link
Member

@AlexPl292 AlexPl292 left a comment

Choose a reason for hiding this comment

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

Please check the comments

@jphalip
Copy link
Contributor Author

jphalip commented Nov 23, 2024

I've improved the implementation to support unicodes, emojis, etc. and added multiple other tests.

However, maybe this is overachieving? For example, standard vim appears to ignore escaping of emojis:

Standard vim:

:echo escape("😀🎉✨", "😀✨")
😀🎉✨

This implementation:

typeText(commandToKeys("""echo escape("😀🎉✨", "😀✨")"""))
assertExOutput("\\😀🎉\\")

Let me know if you're ok with this or if you'd like to scale back to the standard behavior.

@AlexPl292
Copy link
Member

Oh, sorry, I didn't mean THAT accurate :D
I mentioned 😀 because I thought that iterating over code points (via String.codePoints()) would generally solve the issue. Will working just with codepoints make the escaping more accurate? If not, let's use the original behavior because it looks much simpler and more stable :)

@AlexPl292 AlexPl292 closed this Dec 4, 2024
@AlexPl292 AlexPl292 reopened this Dec 4, 2024
@AlexPl292
Copy link
Member

Note: I closed & reopened the PR by accident

@jphalip
Copy link
Contributor Author

jphalip commented Dec 5, 2024

@AlexPl292 I've reverted to my original, simpler implementation. I also added some tests for unicode, and verified that the results are the same in standard Vim.

@AlexPl292
Copy link
Member

Great, thank you!

@AlexPl292 AlexPl292 merged commit 2e550a0 into JetBrains:master Dec 6, 2024
4 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.

3 participants