-
Notifications
You must be signed in to change notification settings - Fork 7.9k
GH-15976: clarify error messages for enum/trait/interface/alias names #15977
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
Conversation
So that it is clearer what changes
Instead of always saying that a name is reserved or deprecated and cannot/should not be used as a class name, take the usage into account and say the name cannot be used as an enum name, trait name, etc. In the process, for class names add a missing "a".
a176aff
to
9fe9001
Compare
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.
Seems reasonable to me, maybe @TimWolla wants to have a look as IIRC they have good opinions about error messages
@@ -3571,7 +3571,7 @@ ZEND_API zend_result zend_register_class_alias_ex(const char *name, size_t name_ | |||
zend_str_tolower_copy(ZSTR_VAL(lcname), name, name_len); | |||
} | |||
|
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.
Unrelated: love the TODO message about moving something out in 7.4. :D
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.
No strong feelings, but this is indeed an improvement.
@DanielEScherzer, I've just noticed that your commits are not signed. While this is, to my knowledge, not a requirement, it might still be a good idea to do it; see https://wiki.php.net/vcs/commit-signing for details. Also, seeing the number of your contributions, consider to apply for a php.net account at https://www.php.net/git-php.php. |
Follow-up to php#15977
It might be a good idea, but that page says as a prerequisite:
which is not the case for me
Given that I've only been contributing for less than a month, I think I'd rather get some more experience with the various undocumented conventions (or if they are documented, can you point me to them?) - e.g., I would have considered #15832 a trivial change and clear improvement, but apparently having the |
That's wrong, you don't need that to sign commits. However, it's also worth noting that most likely your commit will either be squashed/rebased by the GitHub GUI, or by the merged in the CLI, in which case your signature is irrelevant anyway. https://github.com/php/php-src/commits/master/?author=DanielEScherzer The reason your last commits aren't signed is because @Girgias' commits aren't signed. /cc @cmb69 |
The documentation about this is seriously lacking. Consider to contribute to https://github.com/php/php-src/tree/master/docs (but note that a couple of things are already documented at https://github.com/php/php-src/tree/master/docs-old, and maybe elsewhere).
Ah, right, although some are. |
Its on my list, #15954 |
It depends on my computer I think. If I remember my setups correctly, I set up the signed commits on my desktop but not on my laptop :| |
Follow-up to php#15977
Instead of always saying that a name is reserved or deprecated and
cannot/should not be used as a class name, take the usage into account and say
the name cannot be used as an enum name, trait name, etc. In the process, for
class names add a missing "a".
Add tests for the new messages.