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

Don't highlight backslash-escaped brackets #777

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rolandwalker
Copy link

@rolandwalker rolandwalker commented Nov 7, 2020

I've looked at #532, #533, #455, and am not entirely sure of your current thinking on this.

This PR addresses only backslash escaping. Current highlighting:
last image

with patch:
Screen Shot 2020-11-07 at 2 49 28 PM

Let me know if this is agreeable, and if so I will figure out how to add a test. Nice setup — tests/generate.zsh is easy.

This coding choice is idiosyncratic:

if (( escaped_state )); then
  :

but if this PR is accepted it will make a cleaner diff with my next one.

@rolandwalker rolandwalker force-pushed the RW/respect-backslash-escaped-brackets branch from 49d5806 to 4662324 Compare November 7, 2020 20:45
@phy1729
Copy link
Member

phy1729 commented Nov 9, 2020

I'm curious what the real world use case is that motivated this PR. A difference between '[' and \[ feels wrong, but I'm not a user of the brackets highlighter and only special casing backslash seems like a potentially reasonable middle ground to those who prefer brackets in quotes be highlighted.

On the PR itself just a few nits. L50 the variables are sorted alphabetically; please keep them in order. For the test I know the script uses $' quoting for BUFFER, but I think due to the number of backslashes and lack of single quotes, simple single quoting would be easier to read. Finally please sort the expectations; the unsorted=1 line will skip the in-order check and sorted is easier for humans to parse.

Other than those minor points the PR looks technically good. The question is should backslash escaped brackets be ignored.

@danielshahaf
Copy link
Member

but if this PR is accepted it will make a cleaner diff with my next one.

What would your next PR do?

@rolandwalker
Copy link
Author

rolandwalker commented Nov 9, 2020

@phy1729 Thank you for the review! The motivation was that I have been writing a lot of Lisp at the command-line, using tools like babashka, simple example

ls | bb -i '(filter #(-> % io/file .isDirectory) *input*)'

That is why it started to make sense to me to balance brackets within strings, though I think it would be better if each string was isolated and treated independently.

@danielshahaf I would like to add another bracket-highlighting style which highlights the balanced pair of containing brackets for any point the cursor may be on.

@rolandwalker rolandwalker force-pushed the RW/respect-backslash-escaped-brackets branch from 4662324 to 12b648e Compare November 9, 2020 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants