Skip to content

docs: Add allowed values of tags in the phpdoc_align#7120

Merged
Wirone merged 1 commit intoPHP-CS-Fixer:masterfrom
kayw-geek:docs/update-phpdoc-align
Jul 21, 2023
Merged

docs: Add allowed values of tags in the phpdoc_align#7120
Wirone merged 1 commit intoPHP-CS-Fixer:masterfrom
kayw-geek:docs/update-phpdoc-align

Conversation

@kayw-geek
Copy link
Copy Markdown
Contributor

@kayw-geek kayw-geek commented Jul 5, 2023

The current document lacks configurable values, which makes it inconvenient for users to configure.

@kayw-geek kayw-geek changed the title DOCS: Add allow values of tags in the phpdoc_align doc docs: Add allow values of tags in the phpdoc_align doc Jul 5, 2023
@kayw-geek kayw-geek force-pushed the docs/update-phpdoc-align branch from 43f858f to 2314e20 Compare July 5, 2023 03:29
@kayw-geek kayw-geek marked this pull request as draft July 5, 2023 03:47
@kayw-geek kayw-geek force-pushed the docs/update-phpdoc-align branch from 2314e20 to 160ae3f Compare July 5, 2023 08:03
@kayw-geek kayw-geek marked this pull request as ready for review July 5, 2023 08:04
@kayw-geek kayw-geek force-pushed the docs/update-phpdoc-align branch from 160ae3f to c552659 Compare July 5, 2023 08:37
Wirone
Wirone requested changes Jul 5, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@Wirone Wirone left a comment

Choose a reason for hiding this comment

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

Thank you @kayw-geek for your contribution but unfortunately it looks like it can't be merged in this form. Allowed values for tags in phpdoc_align are not specified by design, because you can pass any tags there:

image

Tags that you have grouped into allowed values set are defined inside fixer to determine the type of tag (@tag <hint> <$var> vs @tag <hint> vs @tag <hint> <signature>, all with optional <desc>) and only represent known tags for each format. It is not complete set of allowed values. Even though it could be helpful for some people, others could be confused.

What we could do is to include supported tags (set currently used for allowed values) and include it somehow in the tags description, like it was done in #6360 for ordered_class_elements fixer. We could even display those tags grouped into sections ("tags with name", "tags with method signature") and describe how it works. If you want to help with this, then let's continue here, but if you don't want do such changes, then let's close this PR 🙂.

@kayw-geek
Copy link
Copy Markdown
Contributor Author

kayw-geek commented Jul 6, 2023 •
edited
Loading

@Wirone
Thank you for your review.
I now have some doubts that the original author's original intention to prohibit users from repairing custom tags?
Is it the right way to restrict users to configure custom tags?

Since the functionality can support fixing all tags, why should we limit users?

@Wirone
Copy link
Copy Markdown
Member

Wirone commented Jul 6, 2023

It's a technical limitation of how phpDoc is parsed with regex. There are some annotations that have type, variable name (and optional description), there are other with only 1 value or with different parts. Fixer allows providing any tags to align, but only some of them are categorised and handled "properly". We could add ability to provide types for custom tags (so you would be able to say that @foo contains signature, like @method), but since you want to only change documentation for current behavior, then we need to document how it works right now.

@kayw-geek kayw-geek force-pushed the docs/update-phpdoc-align branch 4 times, most recently from 0523ca0 to b7aada7 Compare July 18, 2023 07:34
@kayw-geek
Copy link
Copy Markdown
Contributor Author

kayw-geek commented Jul 18, 2023

@Wirone Sorry, some work has delayed the update of this PR.
Can you help review it again if you have time?

This 'CI / PHP 8.2 on macOS (pull_request) ' job should need to try run

@kayw-geek kayw-geek requested a review from Wirone July 18, 2023 08:12
Wirone
Wirone reviewed Jul 18, 2023
View reviewed changes
Comment thread src/Fixer/Phpdoc/PhpdocAlignFixer.php Outdated
@kayw-geek kayw-geek force-pushed the docs/update-phpdoc-align branch 2 times, most recently from 70e71c8 to 3d2cf52 Compare July 18, 2023 09:01
@kayw-geek kayw-geek requested a review from Wirone July 18, 2023 09:16
Wirone
Wirone reviewed Jul 18, 2023
View reviewed changes
Comment thread src/Fixer/Phpdoc/PhpdocAlignFixer.php Outdated
@kayw-geek kayw-geek force-pushed the docs/update-phpdoc-align branch 2 times, most recently from c788d17 to d724e6e Compare July 18, 2023 09:44
@kayw-geek kayw-geek requested a review from Wirone July 18, 2023 10:03
@kayw-geek kayw-geek force-pushed the docs/update-phpdoc-align branch from d724e6e to d94866c Compare July 18, 2023 10:13
@Wirone
Copy link
Copy Markdown
Member

Wirone commented Jul 18, 2023

@kayw-geek you can run auto-review tests locally with docker compose run php-8.2 vendor/bin/paraunit run --group=auto-review, to not trigger CI 🙂.

@kayw-geek kayw-geek force-pushed the docs/update-phpdoc-align branch from d94866c to 9c7083c Compare July 18, 2023 12:47
@kayw-geek
Copy link
Copy Markdown
Contributor Author

kayw-geek commented Jul 18, 2023

@Wirone
I noticed that the code block was not displayed correctly in the description of options in list doc.
Is this the expected behavior?

Do we need to use RstUtils::toRst to convert in other PR?

https://github.com/kayw-geek/PHP-CS-Fixer/blob/9c7083c1c01980d45eb065c84256510112698302/src/Documentation/ListDocumentGenerator.php#L96

@kayw-geek kayw-geek force-pushed the docs/update-phpdoc-align branch from 9c7083c to 8ca41f0 Compare July 18, 2023 13:00
@Wirone
Copy link
Copy Markdown
Member

Wirone commented Jul 18, 2023

@kayw-geek I've checked-out your branch and run dev-tools/doc.php and there are no changes, so it's OK considering current format. If we decide to change how the docs are generated, then we only need to run this script and regenerate docs for all fixers. However, your branch is not rebased on current master:

image

Can you do it, so we're 100% sure everything is fine?

@kayw-geek kayw-geek force-pushed the docs/update-phpdoc-align branch from 8ca41f0 to 655d528 Compare July 18, 2023 14:28
@Wirone Wirone changed the title docs: Add allow values of tags in the phpdoc_align doc docs: Add allowed values of tags in the phpdoc_align docs Jul 18, 2023
@Wirone Wirone changed the title docs: Add allowed values of tags in the phpdoc_align docs docs: Add allowed values of tags in the phpdoc_align Jul 18, 2023
@kayw-geek
Copy link
Copy Markdown
Contributor Author

kayw-geek commented Jul 20, 2023

@Wirone Hi, Does this PR need to be merged after #7161 ?

@Wirone
Copy link
Copy Markdown
Member

Wirone commented Jul 20, 2023

@kayw-geek these are not related. I just had yet another doubt about formatting and thought I could do it myself instead of bothering you again and again, but unfortunately I had no time for it. It's about square brackets inside parentheses, it doesn't make much sense, but I overlooked it somehow 🫠.

@kayw-geek kayw-geek force-pushed the docs/update-phpdoc-align branch from 655d528 to a5e550f Compare July 20, 2023 08:52
@kayw-geek
Copy link
Copy Markdown
Contributor Author

kayw-geek commented Jul 20, 2023

@Wirone So friendly. This is the duty of the PR author.

Wirone
Wirone approved these changes Jul 21, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@Wirone Wirone left a comment

Choose a reason for hiding this comment

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

@Wirone I noticed that the code block was not displayed correctly in the description of options in list doc. Is this the expected behavior?

Do we need to use RstUtils::toRst to convert in other PR?

kayw-geek/PHP-CS-Fixer@9c7083c/src/Documentation/ListDocumentGenerator.php#L96

Now I got what you were talking about, probably. Even though doc/rules/phpdoc/phpdoc_align.rst is OK, doc/list.rst has single backticks so allowed values are displayed differently:
image

However, this is not responsibility of this PR to fix this, so if you're interested in it you could prepare another PR that would fix how these options are rendered in the rules' list 🙂. This PR looks OK 👍.

@Wirone Wirone merged commit 9904dfd into PHP-CS-Fixer:master Jul 21, 2023
@Wirone
Copy link
Copy Markdown
Member

Wirone commented Jul 21, 2023

Thank you @kayw-geek 🍻

@kayw-geek kayw-geek deleted the docs/update-phpdoc-align branch July 24, 2023 02:07
@keradus keradus mentioned this pull request Aug 9, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Wirone Wirone Wirone approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@kayw-geek @Wirone