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

Revert PR - comments format #3555

Closed
sreichel opened this issue Sep 27, 2023 · 10 comments
Closed

Revert PR - comments format #3555

sreichel opened this issue Sep 27, 2023 · 10 comments
Labels

Comments

@sreichel
Copy link
Contributor

Ref #3552

Please check https://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutorial_phpDocumentor.howto.pkg.html#basics.docblock

@sreichel sreichel added the bug label Sep 27, 2023
@pquerner
Copy link
Contributor

Does OpenMage/Magento have a public phpDocumentor archive or why is that important to keep?

@fballiano
Copy link
Contributor

ah ok so this is the documentation (I searched for that before creating the PR and couldn't find):
Screenshot 2023-09-27 alle 16 17 15

In short, it duplicates the doc in the template for all the "vars".

I'd agree to revert if the doc had types but it seems to me that it only has descriptions (https://github.com/OpenMage/magento-lts/pull/3552/files) so I don't see the point of having a template just for a generic description of multiple constants, which don't even explain what the constant does :-\

eg:
Screenshot 2023-09-27 alle 16 20 51
it seems not very useful to me to have a duplicate "names" description for those constants, actually it seems worse than just not having the description at all.

But if people think it's important let's revert it.

@ADDISON74
Copy link
Collaborator

The arguments offered are in favor of a revert. I have no idea how useful will be those comments in the future.

@fballiano
Copy link
Contributor

The arguments offered are in favor of a revert. I have no idea how useful will be those comments in the future.

@ADDISON74 actually I said the opposite 😅 and from the emoj reaction I think @elidrissidev agree. @pquerner just asked a question. so let's see if somebody else participates in the discussion.

@elidrissidev
Copy link
Member

I do not oppose reverting the PR as @sreichel's point is completely valid, but the comments that were removed are not adding much value anyways and the description of the constants as well as their type can be easily inferred from their names. Anyways I'll approve it if someone creates it.

@fballiano
Copy link
Contributor

fballiano commented Sep 27, 2023

I don't think there's any value in reverting so I'll not participate, but I'll not oppose, obviously.

edit: actually I think the source is far less readable and it would be so much better to have single comments for every const/variable, but with proper description/typing, otherwise what's the point.

@Flyingmana
Copy link
Member

What I see its a functionality to copy comments onto multiple variables? My first question would be, is it supported by phpstan for type informations? (or phpstorm?)
But even then I would prefer not having it, as its an uncommon notation many will not understand the meaning of.
But it means we would need to modify the changed files again, as we lost some type and some deprecated annotations (which were intended for multiple entries.
And its a bit odd to have the comments in plural, when the comment is copied into each entry.

Its also documented in their code, although not in their documentation https://github.com/phpDocumentor/ReflectionDocBlock/blob/7b217217725dc991a0ae7b995041cee1d5019561/src/DocBlock.php#L98-L117

@fballiano
Copy link
Contributor

I can create a PR in like 15 minutes to convert those few "real" tags that we lost (1 deprecated and a couple of "mixed" types) if wanted

@sreichel
Copy link
Contributor Author

sreichel commented Sep 27, 2023

Does OpenMage/Magento have a public phpDocumentor archive or why is that important to keep?

There is no public one, but maybe private?


With #3552 phpDocumentor looses some descriptions - for absolutly no reason.

I am also pretty sure we had a similar PR years ago, that was closed for same reason. (cant find it)

@sreichel
Copy link
Contributor Author

Closed with #3558

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants