Skip to content

Conversation

@samsonasik
Copy link
Member

Add @var to Class Constants with VarConstantCommentRector rector rule.

Checklist:

  • Securely signed commits

@samsonasik samsonasik merged commit b0cf1f3 into codeigniter4:develop Jun 1, 2021
@samsonasik samsonasik deleted the apply-rector-var-constant-comment branch June 1, 2021 10:54
@samsonasik
Copy link
Member Author

@paulbalandan thank you for the review.

@MGatner
Copy link
Member

MGatner commented Jun 1, 2021

I realize I missed the boat on this, but this seems unnecessary. Class constants are constant, so their type is always apparent from their declaration. What does adding docblocks accomplish?

@samsonasik
Copy link
Member Author

@MGatner that mostly for consistency with other constants that has @var

@MGatner
Copy link
Member

MGatner commented Jun 1, 2021

I would rather see a rule that pulled all @var tags from constants. "var" is shorthand for "variable" (literally "values that change") and is decidedly not "constant". In addition to being unnecessary I suspect this is probably a docblock violation (though I haven't researched that).

@samsonasik
Copy link
Member Author

This is example in phpDocumentor for using @var for const.

https://github.com/phpDocumentor/phpDocumentor/blob/afecb1227c943f31633ef4587769fbdfa7e47cc7/tests/data/ApiTagTest.php#L65-L74

I will try create new custom rule for pull @var for Class Constants if we have decision is to remove them.

@MGatner
Copy link
Member

MGatner commented Jun 1, 2021

I do see that constants can have @var tags (also found this https://github.com/phpDocumentor/phpDocumentor/blob/dbbe6c48e3553870f3077d8dcc2eda57a1df2c3b/src/phpDocumentor/Descriptor/ConstantDescriptor.php#L109-L123). But does that mean they should? All I can find on that topic is debates on StackOverflow 😅

I'd say at this point it is up to us. I won't fight it if you all want them there, doesn't hurt anything.

@samsonasik
Copy link
Member Author

@MGatner ok, here PR to remove it #4766

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.

3 participants