Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Open
Kerigard wants to merge9 commits intobarryvdh:master
base:master
Choose a base branch
Loading
fromKerigard:feat/separate-tags

Conversation

@Kerigard
Copy link

@KerigardKerigard commentedSep 17, 2022
edited
Loading

Summary

This PR adds support for thelaravel_phpdoc_separation rule 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

  • 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 viacomposer fix-style

daniel-de-wit reacted with thumbs up emoji
@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

@mfnmfn 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?

@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.

@KerigardKerigard requested a review frommfnFebruary 26, 2023 11:43
Copy link
Collaborator

@mfnmfn 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 fixeralways after any kind of generate ran (also e.g. rector) is simply a requirement?


####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 <markus@fischer.name>
@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 commentedFeb 27, 2023
edited
Loading

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 freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@mfnmfnmfn requested changes

+1 more reviewer

@faytekinfaytekinfaytekin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

PHPDoc generation for models violateslaravel_phpdoc_separation StyleCI rule

4 participants

@Kerigard@michaeljhopkins@mfn@faytekin

[8]ページ先頭

©2009-2025 Movatter.jp