Skip to content

Operator highlight extended and improved #50

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

Merged

Conversation

Hi-Angel
Copy link
Member

@Hi-Angel Hi-Angel commented May 4, 2025

Prior to this PR some operators weren't correctly highlighted. One example is the tuple construction "1 /\ 2", it has weird "half-highlight", because it's not covered by operators and then gets caught by some other rule.

With this PR such highlight is hopefully fixed. For inspiration, I looked at the regexp from the official Atom purescript highlight. It works a bit differently: it excludes chars from class. AFAIK Emacs regexps can't do that, so instead I just enlisted the symbols. One thing I did differently though is including colon, because it is a valid operator in PureScript (for List), I think Atom has a bug in their regexp.

Another change being done is I replaced the underlying face from variable-name-face to builtin-face. It is open for debate which face is the best, but variable-name is just wrong because it's far from what's actually being highlighted; and the face is used e.g. by color-identifiers-mode to find identifiers (variables or constants), which operators are not.

What face is the most appropriate is arguable. Emacs 29.1 has
"operator-face", but it's unavailable for older Emacs, and even for
newer versions it's just "nil", so no highlight. Kinda pointless face.

The current "variable-name" is definitely wrong though, because
operators are far from being variables. And there's a separate
"color-identifiers-mode" which uses this face to highlight similar
variables, and that would work wrong for purescript-mode.

Thus, replace the highlight to "builtin".
@Hi-Angel Hi-Angel force-pushed the font-lock-operator-improvements branch from 1fb5e59 to c84353e Compare May 4, 2025 10:32
@Hi-Angel
Copy link
Member Author

Hi-Angel commented May 4, 2025

CC: @niklas hi, I know you're using the mode, so I'm pretty sure you've seen the mishighlight, like with the Tuple. So, just saying this PR is pending to fix the problem. If you do PS devel, you might want to try the branch out. I will be using the changes too locally, but unfortunately lately at my work I'm moving away from PureScript, so not sure how well I'd test it.

Either way, I wrote some tests too, hopefully I didn't miss anything 😊

For example, the tuple construction `1 /\ 2` was weirdly
half-haighlighted before this commit, because it wasn't covered by the
"operators regexp" and was getting caught by some other rule. Fix that.

For inspiration, I looked at the regexp from the official Atom
purescript highlight
https://github.com/purescript-contrib/atom-language-purescript/blob/d17eee55b12c140e8a1a750cce7e5aa9b09653c2/src/purescript.coffee#L32
that one works a bit differently, it excludes chars from class. AFAIK
Emacs can't do that, so instead I just enlisted the symbols. One thing
I did differently though is including colon, because it is a valid
operator in PureScript (for List), I think Atom has a bug in their
regexp.
@Hi-Angel Hi-Angel force-pushed the font-lock-operator-improvements branch from c84353e to 49e0fae Compare May 4, 2025 10:42
Hi-Angel added 2 commits May 5, 2025 11:24
The (,) and (->) don't have much use in PureScript, nor in Haskell for
that matter. In Haskell (,) was a prefix notation for tuple
construction, but in PureScript this operator isn't a thing. Both
trigger "syntax error" in PureScript (and (->) doesn't work even in
Haskell).

Indeed a debatable rule. Let's just remove it.
@Hi-Angel
Copy link
Member Author

Hi-Angel commented May 9, 2025

Okay, I've used it for a while, everything seems fine. Merging.

@Hi-Angel Hi-Angel merged commit 0f7b4ad into purescript-emacs:master May 9, 2025
5 checks passed
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