Skip to content

Conversation

@piotrgradzinski
Copy link
Contributor

Make PHPDoc for enum getters/setters correspond to enum class.

Fixes for #23385.

@piotrgradzinski piotrgradzinski requested a review from a team as a code owner November 19, 2025 16:28
@piotrgradzinski piotrgradzinski requested review from bshaffer and removed request for a team November 19, 2025 16:28
@bshaffer bshaffer added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Nov 19, 2025
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Nov 19, 2025
Copy link
Contributor

@bshaffer bshaffer left a comment

Choose a reason for hiding this comment

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

This looks great but I would also like to see some test of the artifacts. We can achieve that by either running php/generate_descriptor_protos.sh and updating the core gencode in the PHP library, and/or by creating a dedicated test which uses PHP Reflection to verify the typehints / phpdoc.

edit: after generating these locally, the links are relative, but they should be absolute. For example:

- @return int one of the values in {@see Google\Protobuf\Syntax}
+ @return int one of the values in {@see \Google\Protobuf\Syntax} 

@bshaffer bshaffer added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Nov 20, 2025
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Nov 20, 2025
Copy link
Contributor

@bshaffer bshaffer left a comment

Choose a reason for hiding this comment

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

This looks great! I think we want to revert the changes made to the wkt.inc file, but otherwise this looks perfect.

One other thought I had is, do we think it would be better to say {@see \Some\Enum::*}? I think that may make it more clear that we're referring to the constants in that class, but I am not sure PHPDoc or other tools are meant to handle that syntax, so it's probably best to leave it as-is.

@bshaffer bshaffer added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Nov 20, 2025
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Nov 20, 2025
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