-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat: add separation of tags into groups #1377
base: master
Are you sure you want to change the base?
Conversation
Would love for this to get merged in. We use Pint in our pipeline and makes ide-helper a bit of a pain |
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?
Fine. I'll make changes |
@mfn Made changes. Please check the README file as I used google translate. |
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 fixer always after any kind of generate ran (also e.g. rector) is simply a requirement?
@@ -263,6 +263,33 @@ add support for creating a new dedicated class instead of using local scopes in | |||
|
|||
If for some reason it's undesired to have them generated (one for each column), you can disable this via config `write_model_external_builder_methods` and setting it to `false`. | |||
|
|||
#### 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 <[email protected]>
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. |
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. |
Summary
This PR adds support for the
laravel_phpdoc_separation
rule for PHPDoc. This feature is disabled by default to avoid problems in existing projects.Fixes #1288
First you need to merge barryvdh/ReflectionDocBlock#8
Type of change
Checklist
composer fix-style