Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork1.2k
feat: add separation of tags into groups#1377
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
michaeljhopkins commentedOct 27, 2022
Would love for this to get merged in. We use Pint in our pipeline and makes ide-helper a bit of a pain |
mfn left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Approach LGTM!
I suggest to:
- fix the merge conflicts
- adapt the tests due to recent changes
- adjust the README to mention the config flag
- rename the config
WDYT?
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Kerigard commentedFeb 25, 2023
Fine. I'll make changes |
Kerigard commentedFeb 26, 2023
@mfn Made changes. Please check the README file as I used google translate. |
mfn left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM, but the I started to question the whole thing why we need to include code style adherence into such a library, wouldn't it make much more sense to adapt the workflow?
Because, yeah well lit may be the official laravel/pint, but people may have different ideas or code styles and it seems just running your style fixeralways after any kind of generate ran (also e.g. rector) is simply a requirement?
Uh oh!
There was an error while loading.Please reload this page.
| ####Separating docblock tags into groups | ||
| By default, all docblock tags are written sequentially on each line. Problems can arise when using Laravel Pint or other linters because they use different formatting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
But reading this, it just occurred to me: does this whole PR even make sense?
Wouldn't it make much more sense to just say:
- well, run ide-helper first
- then your favourite code style fixer
?
It reduces complexity on this library, which shouldn't be much concerned with styling anyway.
WDYT?
Co-authored-by: Markus Podar <markus@fischer.name>
Kerigard commentedFeb 27, 2023
Not everyone has automated code styling processes set up. In such situations, the user may simply forget to run the linter and push all changes to git. |
mfn commentedFeb 27, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I can't argue with that, I don't have numbers to prove it. But I'm taking a step back and thinking: why add flexibility of code formatting to a library whose goal is not to format the code? The dedicated tools are there for a reason and are much better equipped to handle this. My argument is: if someone comes up with the idea he's not satisfied with how this library generates the code "by default",then it is time to think about adding automatic code formatting to your project. It seems backward adjusting libraries when we've the right tools already. I'm sorry for the forth and back here and time you invested, but from my side, now that I properly understand the intention here, it's 👎🏼 to merge this PR with its current goal. |
Uh oh!
There was an error while loading.Please reload this page.
Summary
This PR adds support for the
laravel_phpdoc_separationrule for PHPDoc. This feature is disabled by default to avoid problems in existing projects.Fixes#1288
First you need to mergebarryvdh/ReflectionDocBlock#8
Type of change
Checklist
composer fix-style