-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Make false, null and true to be recognized as constants in PHP #1694
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get that null and booleans should have the class constant
but why also predefined-constant
?
This class is not supported by Prism's themes and it does not capture other predefined constants. (I don't want to suggest that we should try to match all predefined constants.)
Also, is the null
class really necessary?
components/prism-php.js
Outdated
@@ -28,6 +28,13 @@ | |||
} | |||
}); | |||
|
|||
Prism.languages.insertBefore('php', 'class-name', { | |||
'boolean': { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't you just overwrite clike
's boolean
in extend
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't you just overwrite clike's boolean in extend?
I think the boolean
regex should be executed before class-name
, because a boolean can’t be a class/trait name (see the table in #1693).
Currently, Prism converts
<?php
class true
{
}
to
<span class="token keyword">class</span> <span class="token class-name">true</span>
<span class="token punctuation">{</span>
<span class="token punctuation">}</span>
Overwriting boolean
from clike
doesn’t fix it.
Using insertBefore()
I get
<span class="token keyword">class</span> <span class="token boolean constant predefined-constant">true</span>
<span class="token punctuation">{</span>
<span class="token punctuation">}</span>
Anyway, I can make changes if this behaviour is intentional. Maybe I overdid this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with null
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will say, conceptually, Prism is not a strict linter. My understanding is class true
in PHP would be a syntax error, so I'd be more concerned here w/ reducing the amount of code we ship than getting the highlighting exactly right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I can keep it simple :)
Prism.languages.php = Prism.languages.extend('clike', {
// ...
'constant': [
{
pattern: /\b[A-Z_][A-Z0-9_]*\b/
},
{
pattern: /\b(?:null)\b/i
}
],
'boolean': {
pattern: /\b(?:false|true)\b/i,
alias: 'constant'
},
// ...
});
false
and true
get token boolean constant
.
null
loses keyword
and gets token constant
.
Is it better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class true
in PHP would be a syntax error
Yes.
I will say, conceptually, Prism is not a strict linter.
Right. I definitely tried to make it lint. I’ll fix that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that definitely better.
Just two small nits:
- Please don't write
{ pattern: /.../ }
. Just use the regex directly. - Please position
boolean
beforeconstant
.constant
afterboolean
creates the false impression thatboolean
is matched afterconstant
.
Thank you for contributing! |
Great! Thank you very much for your help! |
Resolves #1693.