8000 GH-15976: clarify error messages for enum/trait/interface/alias names by DanielEScherzer · Pull Request #15977 · php/php-src · GitHub
[go: up one dir, main page]

Skip to content

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

Merged
merged 2 commits into from
Sep 22, 2024

Conversation

DanielEScherzer
Copy link
Member

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.

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".
Copy link
Member
@Girgias Girgias left a 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);
}

Copy link
Member

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

Copy link
Member
@TimWolla TimWolla left a 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.

@Girgias Girgias merged commit 79d708c into php:master Sep 22, 2024
8 of 10 checks passed
@DanielEScherzer DanielEScherzer deleted the name-errors branch September 22, 2024 20:42
@DanielEScherzer
Copy link
Member Author
DanielEScherzer commented Sep 22, 2024

Double quotes change is at #15990, trait test name change is at #15991

@cmb69
Copy link
Member
8000 cmb69 commented Sep 22, 2024

@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.

DanielEScherzer added a commit to DanielEScherzer/php-src that referenced this pull request Sep 22, 2024
@DanielEScherzer
Copy link
Member Author

@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.

It might be a good idea, but that page says as a prerequisite:

You are able to pull and push to at least one PHP repository.

which is not the case for me

Also, seeing the number of your contributions, consider to apply for a php.net account at https://www.php.net/git-php.php.

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 do {} while (0) is a best practice even when it is unneeded. Thanks for the vote of confidence though!

@iluuu1994
Copy link
Member

It might be a good idea, but that page says as a prerequisite:

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

@cmb69
Copy link
Member
cmb69 commented Sep 22, 2024

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 do {} while (0) is a best practice even when it is unneeded.

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).

The reason your last commits aren't signed is because @Girgias' commits aren't signed.

Ah, right, although some are.

@DanielEScherzer
Copy link
Member Author

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 do {} while (0) is a best practice even when it is unneeded.

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).

Its on my list, #15954

@iluuu1994
Copy link
Member

Ah, right, although some are.

Right, those are the PRs merged through the GitHub UI:

ae4ef32

image

Maybe @Girgias accidentally stopped signing commits.

@Girgias
Copy link
Member
Girgias commented Sep 22, 2024

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 :|

DanielEScherzer added a commit to DanielEScherzer/php-src that referenced this pull request Sep 22, 2024
Girgias pushed a commit that referenced this pull request Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
0