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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix TextMate regex #137

Merged
merged 3 commits into from Jun 21, 2023
Merged

Fix TextMate regex #137

merged 3 commits into from Jun 21, 2023

Conversation

glcraft
Copy link
Contributor

@glcraft glcraft commented Jun 20, 2023

From hustcer/nu-grammar#1

Remove non-fixed text width in lookbehind and negative lookahead regex.

Note: about "command" rule regex, i didn't remember why I wrote |(?<=\\)|\\}|\\]|\\$\\w+)\\. in "command" rule. I think it was useful in early stage of making this file.
Anyway, I didn't see any case where it is useful now. I tried different files to see if it causes any issue if I remove it, but I didn't see any.
So if someone see something wrong in the future, let me now 馃憤

@fdncred
Copy link
Collaborator

fdncred commented Jun 20, 2023

I'm not sure this is working right. Here is variables
image

I'm struggling to find something that will test the other changes.

@glcraft
Copy link
Contributor Author

glcraft commented Jun 20, 2023

Argh yeah, that was the issue I was trying to avoid in command... I have to do something so

It surely has been useful in early stage of the syntax
but it's seems to have no impact now
get rid of non-fixed text width lookbehind, incompatible with PCRE regex
variable can have 0 or more fields
@glcraft
Copy link
Contributor Author

glcraft commented Jun 21, 2023

okay, i just fix the issue
I forgot to transform a "one or more" variable fields repetition to "zero of more" in variables rule.

Tell me if you find something wrong 馃憤

@fdncred
Copy link
Collaborator

fdncred commented Jun 21, 2023

I looked it over and tested it out. Nothing else seems to jump out. I did find a new issue though unrelated to this PR. #139

@fdncred fdncred merged commit 18c5541 into nushell:main Jun 21, 2023
1 check passed
@fdncred
Copy link
Collaborator

fdncred commented Jun 21, 2023

Thanks for all your help!

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.

None yet

2 participants