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

PhpdocOrderByValueFixer - Introduction #4569

Conversation

@localheinz
Copy link
Contributor

localheinz commented Oct 5, 2019

This PR

  • extracts a PhpdocOrderByValueFixer
  • deprecates the PhpUnitOrderedCoversFixer
@localheinz localheinz force-pushed the localheinz:feature/php-unit-ordered-annotations branch 2 times, most recently from 6131304 to fb0ae37 Oct 5, 2019
README.rst Outdated Show resolved Hide resolved
@localheinz localheinz marked this pull request as ready for review Oct 5, 2019
@localheinz localheinz force-pushed the localheinz:feature/php-unit-ordered-annotations branch from 86246c9 to 9756cb1 Oct 5, 2019
README.rst Outdated Show resolved Hide resolved
@SpacePossum

This comment has been minimized.

Copy link
Member

SpacePossum commented Oct 9, 2019

I would like to merge this one, so if anyone has more this please let me know otherwise I resolve the minors and continue :)

@localheinz localheinz force-pushed the localheinz:feature/php-unit-ordered-annotations branch 3 times, most recently from 880b5bc to a46a267 Oct 11, 2019
@localheinz

This comment has been minimized.

Copy link
Contributor Author

localheinz commented Oct 11, 2019

@julienfalque @SpacePossum

Perhaps it makes sense to adjust the PhpdocOrderFixer to also take into account a range of other annotations?

I like

  • having annotations in a specific order
  • having annotations grouped (separated by an empty line)

In other words, make the PhpdocOrderFixer configurable!

What do you think?

@SpacePossum

This comment has been minimized.

Copy link
Member

SpacePossum commented Oct 11, 2019

I think that would be great :)
Do you want to incorporate the work done here in the new fixer into that one or keep those separated? It might be easier to have one to prevent conflicts and make the configuration for the end user more easy... hmm... I might need to give it some more thought though, but it does sound good

@localheinz

This comment has been minimized.

Copy link
Contributor Author

localheinz commented Oct 13, 2019

Maybe there’s room for more fixers?

  • group annotations
  • sort annotations within a group
  • sort annotation groups

Personally, I would use all of them!

@localheinz localheinz force-pushed the localheinz:feature/php-unit-ordered-annotations branch from 5a45c6f to cdff945 Oct 13, 2019
@localheinz

This comment has been minimized.

Copy link
Contributor Author

localheinz commented Oct 13, 2019

Build failures are currently related to failing to download static analysis tools.

@localheinz localheinz force-pushed the localheinz:feature/php-unit-ordered-annotations branch 2 times, most recently from f7bbba3 to 870bfc8 Oct 15, 2019
README.rst Outdated Show resolved Hide resolved
@keradus

This comment has been minimized.

Copy link
Member

keradus commented Nov 3, 2019

@localheinz , can you please rebase and handle newest code review ?

@localheinz

This comment has been minimized.

Copy link
Contributor Author

localheinz commented Nov 3, 2019

@keradus

Apologies for the delay - will do!

@localheinz localheinz force-pushed the localheinz:feature/php-unit-ordered-annotations branch 2 times, most recently from 3a74b7f to 06500e7 Nov 3, 2019
@localheinz

This comment has been minimized.

Copy link
Contributor Author

localheinz commented Nov 3, 2019

@julienfalque @keradus @kubawerlos @SpacePossum

I have

  • rebased on top of master
  • added handling of @coversNothing
  • added handling of @internal

and I apologize for the delay.

If you find something else that needs to be addressed, I promise to sort it out quickly!

@localheinz localheinz changed the title Introduce PhpUnitOrderedAnnotationsFixer PhpUnitOrderedAnnotationsFixer - Introduction Nov 7, 2019
@julienfalque julienfalque added this to the 2.17.0 milestone Nov 10, 2019
README.rst Outdated

Configuration options:

- ``annotations`` (a subset of ``['author', 'covers', 'coversNothing',

This comment has been minimized.

Copy link
@localheinz

localheinz Nov 11, 2019

Author Contributor

Just thinking, since this fixer handles annotations which are not bound to phpunit test cases, such as

  • @author
  • @internal

and since we already have PhpdocOrderFixer, perhaps this fixer here should be renamed to PhpdocOrderByValueFixer?

Here's what I think:

Current Fixers

  • PhpdocOrderFixer groups annotations by name (@param, @throws, @return) and inserts an empty line between them

New Fixers

  • PhpdocOrderByValueFixer sorts configured annotations by value
  • PhpdocOrderByNameFixer sorts configured annotations by name, and optionally inserts a blank line between them

Deprecated Fixers

  • PhpdocOrderFixer can be deprecated and proxy to the PhpdocOrderByNameFixer

This comment has been minimized.

Copy link
@localheinz

This comment has been minimized.

Copy link
@julienfalque

julienfalque Nov 11, 2019

Member

That's exactly what I was thinking when reading the changes, 👍 for me. Though I think we can merge this PR (after renaming the new fixer) and work on the other fixers in another PR?

This comment has been minimized.

Copy link
@localheinz

localheinz Nov 11, 2019

Author Contributor

@julienfalque

Sounds good!

Renamed the fixer!

@localheinz localheinz changed the title PhpUnitOrderedAnnotationsFixer - Introduction PhpdocOrderByValueFixer - Introduction Nov 11, 2019
@julienfalque julienfalque force-pushed the localheinz:feature/php-unit-ordered-annotations branch from edeb0a8 to 8c0a85f Nov 19, 2019
@julienfalque

This comment has been minimized.

Copy link
Member

julienfalque commented Nov 19, 2019

Thank you @localheinz.

julienfalque added a commit that referenced this pull request Nov 19, 2019
This PR was squashed before being merged into the 2.17-dev branch (closes #4569).

Discussion
----------

PhpdocOrderByValueFixer - Introduction

This PR

* [x] extracts a `PhpdocOrderByValueFixer`
* [x] deprecates the `PhpUnitOrderedCoversFixer`

Commits
-------

8c0a85f PhpdocOrderByValueFixer - Introduction
@julienfalque julienfalque merged commit 8c0a85f into FriendsOfPHP:master Nov 19, 2019
1 of 4 checks passed
1 of 4 checks passed
ci/circleci Your tests are queued behind your running builds
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details
@julienfalque julienfalque removed the RTM label Nov 19, 2019
@localheinz localheinz deleted the localheinz:feature/php-unit-ordered-annotations branch Nov 19, 2019
@localheinz

This comment has been minimized.

Copy link
Contributor Author

localheinz commented Nov 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.