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

Rec/quoted quote #143

Merged
merged 14 commits into from
Sep 17, 2020
Merged

Rec/quoted quote #143

merged 14 commits into from
Sep 17, 2020

Conversation

recri
Copy link
Contributor

@recri recri commented Sep 8, 2020

I think this solves #142 by simply rewriting julia-char-regex. The regex didn't actually recognize '\'' as a character constant as it was written. It could still be wrong. Added a test for my particular case.

Sorry, I should have rebased or something to squash the intermediate commits.

Copy link
Contributor

@non-Jedi non-Jedi left a comment

Choose a reason for hiding this comment

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

If the tests I suggested are added and pass, this LGTM. Thanks for working on fixing this! :)

julia-mode.el Outdated
"\\\\")
(or (not (any "\\'"))
(seq "\\" (or (repeat 1 9 (not (any "\\'")))
(any "\\'"))))
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this match e.g. '\n', \0, \x7f, \177? It doesn't seem like it would. Probably worth adding a test for those.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They all fit into the "(1 to 9 (not (backslash or singlequote))) after backslash" clause. I could be more prescriptive, if you like.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense. This is fine. If you wanted to, you could add a couple comments with more explanation of the different forms of char literals and how they're matched, but I don't think that should block merging this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, explanations would be great if @recri has the time. Also, indentation of (seq ... looks a bit off.

@@ -482,6 +482,18 @@ identity"
a = \"#\" # |>
identity"))

(ert-deftest julia--test-indent-quoted-single-quote ()
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add julia--should-font-lock tests for examples from #142.

@@ -620,6 +632,10 @@ identity"))
(julia--should-font-lock s1 4 nil)
(julia--should-font-lock s1 10 nil)))

(ert-deftest julia--test-char-const-font-lock ()
(dolist (c '("'\\''" "'\\\"'" "'\\\\'" "'\\010'" "'\\xfe'" "'\\uabcd'" "'\\Uabcdef01'" "'\\n'" "'\\alpha'" "'a'" "'z'"))
(julia--should-font-lock c 1 font-lock-string-face)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to test that the string-face ends at the end of the char literal.

julia-mode.el Outdated
Comment on lines 113 to 121
(or ; two alternatives
(not (any "\\'")) ; one character, not single quote or backslash
(seq "\\" ; sequence of a backslash followed by ...
(or ; four alternatives
(any "\\'\"?abfnrtv") ; single character escape
(repeat 1 3 (any "0-7")) ; octal escape
(seq 'x' (repeat 1 8 hex)) ; hex escape
(seq 'u' (repeat 1 4 hex)) ; unicode escape
(seq 'U' (repeat 1 8 hex))))) ; extended unicode escaple
Copy link
Collaborator

@tpapp tpapp Sep 15, 2020

Choose a reason for hiding this comment

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

Suggested change
(or ; two alternatives
(not (any "\\'")) ; one character, not single quote or backslash
(seq "\\" ; sequence of a backslash followed by ...
(or ; four alternatives
(any "\\'\"?abfnrtv") ; single character escape
(repeat 1 3 (any "0-7")) ; octal escape
(seq 'x' (repeat 1 8 hex)) ; hex escape
(seq 'u' (repeat 1 4 hex)) ; unicode escape
(seq 'U' (repeat 1 8 hex))))) ; extended unicode escaple
(or ; two alternatives
(not (any "\\'")) ; one character, not single quote or backslash
(seq "\\" ; sequence of a backslash followed by ...
(or ; four alternatives
(any "\\'\"?abfnrtv") ; single character escape
(repeat 1 3 (any "0-7")) ; octal escape
(seq 'x' (repeat 1 8 hex)) ; hex escape
(seq 'u' (repeat 1 4 hex)) ; unicode escape
(seq 'U' (repeat 1 8 hex))))) ; extended unicode escaple

(suggested indentation)

@recri
Copy link
Contributor Author

recri commented Sep 15, 2020

Sorry, I seem to have crossed the git gods somehow. No, it was the emacs gods, running tests with out of date .elc.

@tpapp
Copy link
Collaborator

tpapp commented Sep 16, 2020

Thanks for all the work! LGTM, I will merge when @non-Jedi approves.

julia-mode.el Outdated
(group "'"))))
(group "'") ; start single quote of character constant
(or ; two alternatives
(not (any "\\'")) ; one character, not single quote or backslash
Copy link
Contributor

Choose a reason for hiding this comment

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

''' is a valid Char constant for '\''. This doesn't match that does it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Amazing, that must have been the only quoted single quote that the original code recognized, and the only one that's not documented in the julia manual.

This last pull also tightens up the \x syntax, julia only permits two hex digits, C permits as many as you write though not guaranteeing what will happen to the excess..

Copy link
Contributor

@non-Jedi non-Jedi left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM.

@tpapp tpapp merged commit 8ea90c7 into JuliaEditorSupport:master Sep 17, 2020
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