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

(swift) Support operator and precedencegroup declarations. #2938

Merged
merged 2 commits into from Jan 6, 2021
Merged

(swift) Support operator and precedencegroup declarations. #2938

merged 2 commits into from Jan 6, 2021

Conversation

svanimpe
Copy link
Contributor

This was the final thing on my list :)

  • Adds support for title highlighting in an operator declaration.
  • Same for a precedencegroup declaration.
  • Highlights the contextual keywords in a precedencegroup. These keywords were removed from the global list because they're very rarely used and would cause mostly incorrect highlights as global keywords.

Comment on lines 287 to 337
const OPERATOR_DECL = {
beginKeywords: 'operator',
contains: [
{
match: /\s+/,
relevance: 0
},
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const OPERATOR_DECL = {
beginKeywords: 'operator',
contains: [
{
match: /\s+/,
relevance: 0
},
const OPERATOR_DECL = {
beginKeywords: 'operator',
ends: hljs.MATCH_NEVER,
contains: [

I'm not sure what to name it (never isn't quite right) but for things like this rather than all that muck to eat spaces it should be easier to just write "leave the mode open until you find a token"... or perhaps never never is clearer than I think?

Copy link
Member

Choose a reason for hiding this comment

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

I'd also be open to a new attribute to suggest this if it was clear and easy to read. But it would really just be sugar for the above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is MATCH_NEVER just a regex that never matches, such as \b\B? I guess it's a bit confusing because it makes it seems as though the mode never ends. But I assume you'd have to combine this with a child mode that ends its parent?

Copy link
Member

Choose a reason for hiding this comment

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

Is MATCH_NEVER just a regex that never matches, such as \b\B?

Well, it doesn't exist yet, but we'd add it - and yes, exactly.

I guess it's a bit confusing because it makes it seems as though the mode never ends.

Exactly, unless manually ended by a child... obviously the mode ends upon input termination. My concern is that confusion between "never ends, literally" and "never ends, but by child".

But I assume you'd have to combine this with a child mode that ends its parent?

Yes, as you already have... this would just avoid that extraneous parsing of whitespace.

I very much thing the approach is correct, and obviously we could do it with just \b\B and a comment, but it seems nice to bless the pattern with a proper name. @allejo Any ideas?

src/languages/swift.js Outdated Show resolved Hide resolved
src/languages/swift.js Outdated Show resolved Hide resolved
@joshgoebel
Copy link
Member

Fairly straight-forward. :)

@joshgoebel
Copy link
Member

Please add issue link to the post so the issue can auto-close when we merge. :) Pretty sure we had an issue on this?

@svanimpe
Copy link
Contributor Author

I've made the suggested changes/fixes.

I'll leave the whitespace handling up to you. Perhaps you can already merge this PR and decide on that issue later, as it probably applies to lots of code, not just this PR?

@joshgoebel
Copy link
Member

I'll leave the whitespace handling up to you. Perhaps you can already merge this PR and decide on that issue later, as it probably applies to lots of code, not just this PR?

Yes, I'm happy to fix.

I don't see a rush (no pending release). Often I use PRs as pressure on myself to get new things added. I wouldn't leave this open for weeks/months for such a reason, but if it remains open a few extra days over the holidays and encourages me to add support for never match, then that seems a win win. I'm curious if allejo has any better thoughts for the naming.

@joshgoebel
Copy link
Member

joshgoebel commented Dec 28, 2020

Added the code, we only need the name. :)

  • NO_MATCH
  • NEVER_MATCH
  • WITH_CHILD (doesn't make sense on it's own but end: WITH_CHILD reads nice. Though at that point should we consider endWithChild: true...
  • VIA_CHILD (see above note)
  • ?

Three other grammars at least use this pattern, and we probably encourage it a bit more.

Edit: Actually it's possible you might want a mode to enter and TRULY never end (such as processing a header, then body) - ie the child mode process all the input to EOF... so we shouldn't assume the child mode is going to have a endsWithParent... making NEVER* seems more precise.

Unless we find the perfect name often a comment might still be helpful... I'm only obsessing over the name because once it goes into the public API we're stuck with it. :)

@joshgoebel
Copy link
Member

@allejo Any thoughts on naming? I'm leaning towards just NEVER_RE and being done with it.

@allejo
Copy link
Member

allejo commented Jan 6, 2021

Would this ever be used anywhere else besides end in a mode? If not, I'd lean towards something like ENDS_NEVER so it doesn't end up being used in other places.

const OPERATOR_DECL = {
    beginKeywords: 'operator',
    ends: hljs.ENDS_NEVER,
    // ...
}

Otherwise, if this can be used elsewhere, NEVER_RE sounds good to me

@joshgoebel
Copy link
Member

joshgoebel commented Jan 6, 2021

Would this ever be used anywhere else besides end in a mode? If not, I'd lean towards something like ENDS_NEVER so it doesn't end up being used in other places.

OMG, at first glance I like it a lot. I'll give it a noodle but I'm not sure where else you'd use it. begin: NEVER doesn't make any sense... If I can't think of anything else we'll go with ENDS_NEVER and I'll get this merged.

@joshgoebel
Copy link
Member

joshgoebel commented Jan 6, 2021

@allejo

Oh, but how do you feel about the fact that technically it's wrong? Usually you'd be pairing this with endsParent in a child:

const OPERATOR_DECL = {
    beginKeywords: 'operator',
    ends: hljs.ENDS_NEVER,
    contains: [
      { 
        begin: ...,
        endsParent: true
    ]
}

Does ENDS_NEVER cause confusion? Does the above seem clear or could a reader being be confused?

@allejo
Copy link
Member

allejo commented Jan 6, 2021

I think reading ends: NEVER would cause the same confusion as ENDS_NEVER, no? If we want to go down a more explicit/verbose route, what about ENDED_BY_CHILD?

@joshgoebel
Copy link
Member

joshgoebel commented Jan 6, 2021

I think reading ends: NEVER would cause the same confusion as ENDS_NEVER, no?

This is a good point... LOL. Maybe a more literal:

  • ends: MATCH_NOTHING_RE.
  • ends: MATCH_NEVER_RE.

If we want to go down a more explicit/verbose route, what about ENDED_BY_CHILD?

Because there are sometimes good reasons to never end it at all... like HTTP response body...

@allejo
Copy link
Member

allejo commented Jan 6, 2021

You have my support for ends: MATCH_NOTHING_RE. It's the most accurate since it's purposely not matching anything for the end of a mode but doesn't imply it can never be ended.

@joshgoebel
Copy link
Member

@svanimpe Thanks for all the hard work.

@joshgoebel joshgoebel merged commit 60734e7 into highlightjs:master Jan 6, 2021
@svanimpe
Copy link
Contributor Author

@joshgoebel What is the release schedule for 10.6?

@joshgoebel
Copy link
Member

I just updated the dates... this next week is going to be busy so I'd say next weekend sounds about right.

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

3 participants