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

Improve recognition of constants in PHP #1688

Merged
merged 2 commits into from Jan 2, 2019
Merged

Improve recognition of constants in PHP #1688

merged 2 commits into from Jan 2, 2019

Conversation

ghost
Copy link

@ghost ghost commented Jan 2, 2019

Resolves #1687.

@RunDevelopment
Copy link
Member

I don't think it's a good idea to say that any word which isn't highlighted as anything else should be a constant.

Let's say there is a feature of PHP which Prism doesn't support (yet). All of the keywords/names related to this feature will be highlighted as constants.
Today, these features are type declarations, group use declarations, return types, and type casting just to name a few. These are only partially supported or not at all.

I encourage you to play around with the examples on the PHP.net page and see for yourself what would be highlighted as constants.

But apart from that, isn't it a naming convention to not use lowercase characters for constants?
Even the page you linked to follows it.

@ghost
Copy link
Author

ghost commented Jan 2, 2019

All of the keywords/names related to this feature will be highlighted as constants.

That’s the way PHP behaves in some circumstances, such as echo X. However, I shall agree that there are other contexts, such as function f($x): type {}.

Prism definitely shouldn’t mark unsupported elements as constants.

OK, let’s stick with PSR‑1, not language itself, i. e., underscore as a word separator only, no lower­‑case letters. Should we have single­‑letter constants (X), then? I think we should. It doesn’t affect type declarations, group use declarations, etc.

@RunDevelopment
Copy link
Member

Should we have single­‑letter constants (X), then?

Sure!

@RunDevelopment RunDevelopment merged commit f1026b4 into PrismJS:master Jan 2, 2019
@RunDevelopment
Copy link
Member

Thank you for contributing!

@ghost ghost deleted the bug-with-php-constants branch January 2, 2019 23:04
@ghost
Copy link
Author

ghost commented Jan 2, 2019

Thank you!

But it’s sad that the PR was merged before Travis CI has finished building :(

@RunDevelopment
Copy link
Member

Don't worry about that.
When you look at the detail, you'll see that all tests passed on 2/4 Node version and the other 2 couldn't find this PR (probably because it was merged already).

@ghost
Copy link
Author

ghost commented Jan 3, 2019

OK, thank you!

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.

2 participants