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

feat: add separation of tags into groups #1377

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Kerigard
Copy link

@Kerigard Kerigard commented Sep 17, 2022

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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Misc. change (internal, infrastructure, maintenance, etc.)

Checklist

  • Existing tests have been adapted and/or new tests have been added
  • Add a CHANGELOG.md entry
  • Update the README.md
  • Code style has been fixed via composer fix-style

@michaeljhopkins
Copy link

Would love for this to get merged in. We use Pint in our pipeline and makes ide-helper a bit of a pain

Copy link
Collaborator

@mfn mfn left a 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?

config/ide-helper.php Outdated Show resolved Hide resolved
src/Alias.php Outdated Show resolved Hide resolved
src/Console/ModelsCommand.php Outdated Show resolved Hide resolved
src/Console/ModelsCommand.php Outdated Show resolved Hide resolved
src/Eloquent.php Outdated Show resolved Hide resolved
src/Method.php Outdated Show resolved Hide resolved
tests/Console/ModelsCommand/SeparateTags/Test.php Outdated Show resolved Hide resolved
@Kerigard
Copy link
Author

Fine. I'll make changes

@Kerigard
Copy link
Author

@mfn Made changes.

Please check the README file as I used google translate.

@Kerigard Kerigard requested a review from mfn February 26, 2023 11:43
Copy link
Collaborator

@mfn mfn left a 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?

CHANGELOG.md Outdated Show resolved Hide resolved
@@ -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.
Copy link
Collaborator

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]>
@Kerigard
Copy link
Author

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.
Also, the user can follow the recommendations for code style without using linters, but then he will have to manually make changes to the models each time.

@mfn
Copy link
Collaborator

mfn commented Feb 27, 2023

Not everyone has automated code styling processes set up

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.

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.

PHPDoc generation for models violates laravel_phpdoc_separation StyleCI rule
4 participants