Skip to content

Fix '?#' character-macro syntax highlighting. #26

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

Closed
wants to merge 1 commit into from
Closed

Fix '?#' character-macro syntax highlighting. #26

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Jul 21, 2013

Try any example in which you use the '?#' built-in ASCII value syntax. You will see that the hashtag(#) is treated as a comment, although, this is not the case in Elixir. Also, the two lines which I altered appear to be redundant. The line in elixir-smie must be removed, otherwise, emacs will change the font-face as before, overridding the elixir-mode-font-lock-defaults.

Also, is there any reason why both of these lines existed concurrently?

Thanks.

@antifuchs
Copy link
Contributor

Thanks for the contribution! Bad news: Sadly, this change breaks the tests. The syntax table entry in elixir-smie is there to allow the indentation engine to skip over comments, and since it doesn't know the beginnings of comments anymore, it'll behave pretty randomly. If you want to work on this some more, you can run the tests via M-x elixir-mode-run-tests (this requires ERT, which I think comes with newer emacsen).

I think I'd prefer either a fix that gets rid of the regex instead of an improved syntax table (as you note, emacs will treat a bare # appropriately if it's in the syntax table), or a fix that uses your updated regex and improves the tokenizer that the indentation engine uses. Let me see if I can work something out (-:

Anyway - thanks again, I'll keep this updated if I manage to make progress on this.

antifuchs pushed a commit that referenced this pull request Jul 21, 2013
This introduces a syntax-propertize function that will ferret out
non-constituent ? characters and apply the generic string delimiter
syntax text property to it and the character it escapes. (This handles
?\# sequences too.)

Fixes #26.
antifuchs added a commit that referenced this pull request Jul 21, 2013
This introduces a syntax-propertize function that will ferret out
non-constituent ? characters and apply the generic string delimiter
syntax text property to it and the character it escapes. (This handles
?\# sequences too.)

Fixes #26.
@antifuchs antifuchs mentioned this pull request Jul 21, 2013
@antifuchs
Copy link
Contributor

@swanyboy2 I've made an updated pull req, #28 that should keep indentation correct while fixing fontification of ?# and ?\# and others. If you give the thumbs-up, I'll merge it (timing out in a few days).

I'll close this pull request in favor of #28, but please re-open if anything comes up (:

Oh, and also, sorry for making github say you authored that commit (which I have fixed now); I really should learn how to rebase properly the first time around.

@antifuchs antifuchs closed this Jul 21, 2013
@ghost
Copy link
Author

ghost commented Jul 21, 2013

Okay. I see now. I suppose I shouldn't be surprised that a change to the "SMIE" file would also alter indentation. I suppose it looks right to me. I am new to Emacs(started a week ago), so I would need to spend more time internalizing some of these various strategies that you are employing. In an effort to further learn Emacs/Elisp, would I be able to ask you some questions over IRC or something?

@antifuchs
Copy link
Contributor

No worries, it is a lot to take in (and I'm not yet sure I have completely understood what SMIE does and how it does it exactly, either). I hang out in the freenode #elixir-lang channel, feel free to join & ask anything!

J3RN pushed a commit to J3RN/emacs-elixir that referenced this pull request Apr 24, 2021
* Describe the reasoning behind the fork

Fixes elixir-editors#22

* Remove unnecessary sentence
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.

1 participant