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

[Regular Expressions] fix #332 and [Clojure] fix and add tests #333

Merged
merged 3 commits into from
May 2, 2016

Conversation

keith-hall
Copy link
Collaborator

@keith-hall keith-hall commented Apr 29, 2016

fix #332

@keith-hall
Copy link
Collaborator Author

keith-hall commented Apr 29, 2016

for some reason, pushing a context (using an empty match pattern) directly from the main context seems to cause the first character to not get the correct scope in some situations. The fix is to set the context instead of pushing it.

@wbond
Copy link
Member

wbond commented Apr 29, 2016

Hmm, I think we'll want to do this a different way since this could break if this syntax is embedded in another language (like it is with Clojure right now).

I may have to dig in to the ST source and see what is causing the issue.

@keith-hall
Copy link
Collaborator Author

I may have to dig in to the ST source and see what is causing the issue.

that would be perfect ;)

@keith-hall
Copy link
Collaborator Author

I think it is already broken for Clojure because Regular Expressions uses a push in main, and the Clojure syntax just includes the Regular Expressions main context, and there is no with_prototype or anything to pop it off the stack when the closing " character is reached.

Unfortunately, the Clojure syntax doesn't have any tests, or I would have discovered it while developing my changes. (and it also shows how important it is to run all syntax tests before making a PR, not just the tests for the syntax being worked on.)

Confirmed by checking the scopes in this example I modified from the first "hello world clojure" result in google:

(def hello (fn [] #"Hello world"))

so as it stands, I think we will need to make some changes to the Clojure syntax anyway?

@keith-hall
Copy link
Collaborator Author

It looks like we can work around this ST3 bug by changing the Regular Expressions main context to:

main:
  - include: unexpected-quantifier
  - match: ''
    push: base-literal

instead of:

main:
  - match: ''
    push: [base-literal, unexpected-quantifier-pop]

but it doesn't fix the Clojure syntax

@keith-hall
Copy link
Collaborator Author

@wbond, in case it helps you investigate what is causing the bug in the ST source, I have logged an issue with the minimal steps required to repro here: sublimehq/sublime_text#1190

@keith-hall keith-hall changed the title [Regular Expressions] fix #332 [Regular Expressions] fix #332 and [Clojure] fix and add tests May 1, 2016
@keith-hall
Copy link
Collaborator Author

keith-hall commented May 1, 2016

I've made a fix for Clojure and added tests :)

I played around a bit in http://www.tryclj.com/ to learn enough Clojure in order to do so ;)

@keith-hall keith-hall force-pushed the regex_tests branch 2 times, most recently from 94ee410 to 0a9b2fb Compare May 1, 2016 14:17
i.e. even if regex has unclosed parens
@wbond
Copy link
Member

wbond commented May 2, 2016

Excellent, thanks for figuring out how to fix this!

@wbond wbond merged commit 5866d5f into sublimehq:master May 2, 2016
@keith-hall keith-hall deleted the regex_tests branch May 2, 2016 19:47
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.

[Regular Expressions] First character in Find bar is always meta.literal
2 participants