Skip to content

fix: enable $ in the classAttributes setting #1012

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

Conversation

colelawrence
Copy link

@colelawrence colelawrence commented Jul 4, 2024

We maintain backward compatibility by checking if the existing classAttributes setting is already escaping the dollar sign.

The current workaround for this is to simply escape the $ manually, which is what I can do in the meantime.

2024-07-04 phosphor code-workspace — phosphor (Workspace) at 12 32PM@2x

@thecrypticace
Copy link
Contributor

thecrypticace commented Jul 5, 2024

Because this is fed directly to a regex I'm inclined to leave this as is. I think it's likely people have learned to use / are using regex syntax on purpose here and changing this could be a breaking change for them. Especially with no way to "unescape" the $.

Escaping $ would likely be less of an issue then say ? from a backwards compatibility point of view. No matter what I think we'd want to be consistent about either escaping either all or no regex special characters and escaping all of them here would definitely be a breaking change.

Given that I'm gonna close this but may think about a similar-ish change for a future version of Intellisense (just perhaps with a different approach).

@colelawrence
Copy link
Author

@thecrypticace - if any unescaped $ is used with the current classAttribute, then that simply isn't possibly working for people.

What do you think about updating the documentation to explicitly state that these classAttributes accept RegExp strings? I believe that would help anyone in the future (like myself) understand how to fix this issue.

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.

2 participants