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

NoSuperfluousPhpdocTagsFixer - Remove superfluous phpdocs for typed properties #4713

Closed
wants to merge 1 commit into from

Conversation

@ruudk
Copy link
Contributor

ruudk commented Dec 27, 2019

Changes superfluous PHPdocs like

class Foo {
    /**
     * @var null|Bar
     */
    private ?Bar $bar;
}

to this:

class Foo {
    /**
     */
    private ?Bar $bar;
}
@ruudk ruudk force-pushed the ruudk:typed-properties branch from e9d601d to db7588c Dec 27, 2019
@ruudk ruudk changed the base branch from 2.16 to master Dec 27, 2019
@ruudk ruudk force-pushed the ruudk:typed-properties branch 2 times, most recently from dc180da to cb0cb2e Dec 27, 2019
@ruudk ruudk force-pushed the ruudk:typed-properties branch 4 times, most recently from 33ed9ee to 637190c Dec 30, 2019
@julienfalque

This comment has been minimized.

Copy link
Member

julienfalque commented Dec 31, 2019

Please do not amend/squash commits as it forces reviewers reading the entire diff on every push instead of only reading added changes.

@ruudk

This comment has been minimized.

Copy link
Contributor Author

ruudk commented Dec 31, 2019

@julienfalque Thanks for your review.

Please do not amend/squash commits as it forces reviewers reading the entire diff on every push instead of only reading added changes.

Done, please have a look :)

@julienfalque

This comment has been minimized.

Copy link
Member

julienfalque commented Jan 1, 2020

@keradus @SpacePossum @kubawerlos Do you consider this a whole new feature or an enhancement of the existing scope?

Copy link
Contributor

kubawerlos left a comment

I'd say new feature, especially that the fixer desciption must be updated as well.

@ruudk ruudk force-pushed the ruudk:typed-properties branch from aefb052 to 891c673 Jan 6, 2020
@ruudk

This comment has been minimized.

Copy link
Contributor Author

ruudk commented Jan 6, 2020

Description updated (new commit) + rebased with master

@ruudk

This comment has been minimized.

Copy link
Contributor Author

ruudk commented Jan 8, 2020

@julienfalque @keradus @SpacePossum @kubawerlos Anything I can do to get this merged?

@julienfalque

This comment has been minimized.

Copy link
Member

julienfalque commented Jan 8, 2020

@ruudk You need to update the README file using the dedicated command: php php-cs-fixer readme > README.rst. This should fix CI builds.

Also, this PR will be a new feature once merged. When we add new functionality to existing rules, we usually do so using a new option disabled by default. This way, users that already rely on the rule won't have new changes applied to their code that are likely unexpected. Though in this case I would be 👍 to merge this without option (so always enabled). @SpacePossum @kubawerlos @keradus WDYT?

@ruudk

This comment has been minimized.

Copy link
Contributor Author

ruudk commented Jan 8, 2020

@julienfalque Thanks for giving me the command, I was not aware. Made the change.

@julienfalque julienfalque added this to the 2.17.0 milestone Jan 8, 2020
@ruudk ruudk force-pushed the ruudk:typed-properties branch from f7bf1f0 to 5c1819d Jan 8, 2020
@ruudk

This comment has been minimized.

Copy link
Contributor Author

ruudk commented Jan 8, 2020

I had to do a rebase to get rid of the merge commit, so that was a good opportunity to add everything as one commit.

@ruudk

This comment has been minimized.

Copy link
Contributor Author

ruudk commented Jan 10, 2020

Can we merge this?

@julienfalque

This comment has been minimized.

Copy link
Member

julienfalque commented Jan 10, 2020

The PR looks good, yet our process requires at least two approvals from maintainers before merging important additions/changes.

Also we still need to agree whether:

  • we merge it on 2.15 (enhancement) or master (new feature);
  • if new feature, enabled by default or with a new option.

I think I'd actually merge this as an enhancement rather than a new feature. My reasoning is that the intent of the rule is to remove all superfluous PHPDoc tags. This includes @var tags on typed properties but those didn't exist yet at the time the rule was introduced.

@ruudk

This comment has been minimized.

Copy link
Contributor Author

ruudk commented Jan 13, 2020

@julienfalque I agree that this is an enhancement.

@SpacePossum @kubawerlos @keradus What do you think? Can we merge this?

@ruudk

This comment has been minimized.

Copy link
Contributor Author

ruudk commented Jan 13, 2020

@julienfalque I see that @kubawerlos approved, so we can merge it 🥳

@julienfalque

This comment has been minimized.

Copy link
Member

julienfalque commented Jan 14, 2020

Not so fast :)

Also we still need to agree whether:

  • we merge it on 2.15 (enhancement) or master (new feature);
  • if new feature, enabled by default or with a new option.

This still needs to be decided.

@SpacePossum SpacePossum added RTM and removed RTM labels Jan 14, 2020
@SpacePossum SpacePossum changed the title Remove superfluous phpdocs for typed properties (PHP 7.4) NoSuperfluousPhpdocTagsFixer - Remove superfluous phpdocs for typed properties Jan 14, 2020
@SpacePossum

This comment has been minimized.

Copy link
Member

SpacePossum commented Jan 14, 2020

I'd vote 2.15, no new option, as the fixer should remove superfluous Phpdoc's and this PR makes sure it does on 7.4 for properties

@ruudk

This comment has been minimized.

Copy link
Contributor Author

ruudk commented Jan 14, 2020

@SpacePossum Sounds like a plan. Can you give it a Github Approval? :)

@SpacePossum

This comment has been minimized.

Copy link
Member

SpacePossum commented Jan 14, 2020

I do you better, if you retarget to 2.15 I'll today or tomorrow :)

@ruudk

This comment has been minimized.

Copy link
Contributor Author

ruudk commented Jan 14, 2020

@SpacePossum Ah, sorry, I forgot that I targeted master. Tried to apply this on 2.15 but a lot has changed in that file. I don't want to rewrite this all, since I already spend a lot of time on this, so let's merge it to 2.17 then?

@SpacePossum

This comment has been minimized.

Copy link
Member

SpacePossum commented Jan 14, 2020

I see, can you try 2.16 than? I think for 2.16 and master the fixer is the same

@ruudk ruudk force-pushed the ruudk:typed-properties branch from 5c1819d to a771bfc Jan 14, 2020
@ruudk ruudk changed the base branch from master to 2.16 Jan 14, 2020
@ruudk

This comment has been minimized.

Copy link
Contributor Author

ruudk commented Jan 14, 2020

@SpacePossum Done! :)

@julienfalque

This comment has been minimized.

Copy link
Member

julienfalque commented Jan 14, 2020

I'm 👎 for merging this into 2.16. We should either merge it into 2.15 (current LTS) or master as new feature. 2.16 is for fixing bugs that don't exist on 2.15.

@SpacePossum

This comment has been minimized.

Copy link
Member

SpacePossum commented Jan 14, 2020

the issue here with the difference between 2.15 and master makes it not feasible to target 2.15, so 2.16 is the lowest supported candidate. There is no gain doing the change on master and not on 2.16 as far as I know.

@kubawerlos

This comment has been minimized.

Copy link
Contributor

kubawerlos commented Jan 14, 2020

Tried to apply this on 2.15 but a lot has changed in that file.

That suggest some maybe important changes, so I'd insist to solve this and do merge to 2.15 then

I already spend a lot of time on this

I bet still 100x less that @SpacePossum on maintaining this project ;) and since there are changes between master and 2.15 I imagine merging it to master would bring some conflicts in future.

@julienfalque julienfalque removed this from the 2.17.0 milestone Jan 15, 2020
@SpacePossum

This comment has been minimized.

Copy link
Member

SpacePossum commented Jan 17, 2020

I checked and it is not that hard to get this on 2.15.

@ruudk your changes to the fixer test cases can go in without any additional adjustments needed.
For the fixer itself, copy paste your version of the 2.16 over the 2.15 one; now remove the remove_inheritdoc option from the definition, the private method removeSuperfluousInheritDoc can go as well, remove the allow_unused_params option as well (and both the code samples etc.), leave the rest. Regenerate the README and it should be fine :)
Let me know if this doesn't work for you, than I can do it for you and make sure you keep authorship of the change.

Please make a new branch of the current one for the 2.15 version, this way, when I merge upstream from 2.15 to 2.16 I can resolve the merge conflict easy because we already have a good 2.16 version, thanks!

@ruudk

This comment has been minimized.

Copy link
Contributor Author

ruudk commented Jan 17, 2020

Thanks for helping me on this. I created a new PR that targets 2.15. #4746

SpacePossum added a commit that referenced this pull request Jan 18, 2020
…s (PHP 7.4) (ruudk)

This PR was merged into the 2.15 branch.

Discussion
----------

NoSuperfluousPhpdocTagsFixer - Remove for typed properties (PHP 7.4)

See #4713

This one targets 2.15. I hope I did it correctly. If not, then #4713 still has the 2.16 version.

Commits
-------

abba0f3 Remove superfluous phpdocs for typed properties (PHP 7.4)
@SpacePossum

This comment has been minimized.

Copy link
Member

SpacePossum commented Jan 18, 2020

merged through #4746 , thanks for keeping the PR it helped we dealing with the merge conflicts 👍 :)

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