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

Jump to bottom

Refactor conversion of PHPDoc to type declarations #4591

Merged
merged 1 commit into from May 3, 2021
Merged

Refactor conversion of PHPDoc to type declarations #4591

merged 1 commit into from May 3, 2021

Conversation

julienfalque
Copy link
Member

@julienfalque julienfalque commented Oct 13, 2019 •
edited

This PR:

@nicolas-grekas nicolas-grekas mentioned this pull request Oct 13, 2019
Closed
7 tasks
@julienfalque julienfalque force-pushed the types-expression-class branch 2 times, most recently from 777cb40 to aba296c Compare October 13, 2019 15:45
@julienfalque julienfalque mentioned this pull request Oct 13, 2019
Merged
@julienfalque julienfalque mentioned this pull request Oct 21, 2019
Merged
@kubawerlos
Copy link
Contributor

Could we instead of abstract fixers introduce - as @SpacePossum suggested one day - a namespace of manipulators?

@julienfalque
Copy link
Member Author

julienfalque commented Oct 23, 2019 •
edited

Not sure what you mean exactly?

@kubawerlos
Copy link
Contributor

Similarly like we have Tokenizer we can create Manipulator and store there classes to be called by PhpdocToParamTypeFixer and PhpdocToReturnTypeFixer.

AbstractPhpdocToTypeDeclarationFixer does not have any property has properties that all are in practice constants, thus the class is set of functions that do stuff for the fixers, similar as all analyzers.

@julienfalque
Copy link
Member Author

Indeed, some parts of the new abstract class could probably be extracted to some "analyzer" class, but some are quite specific to these fixers. I'll have a closer look shortly.

@julienfalque julienfalque force-pushed the types-expression-class branch 6 times, most recently from 982d539 to 336a3d4 Compare November 7, 2019 20:16
@keradus
Copy link
Member

keradus commented Nov 24, 2019

as this PR is also fixing a bugfix from 2.16 line, shouldn't it actually target 2.16.x as well ?

@julienfalque
Copy link
Member Author

This PR also introduces new public API in non-internal code (DocBlock) so it should be released in a new minor version according to SemVer. Though these new API are more of an detail of this refactoring than an actual new feature, so I'm fine merging this into 2.16. What do you prefer?

@keradus
Copy link
Member

keradus commented Nov 24, 2019 •
edited

so, you created a bugfix+feature in single PR?
can we backport bugfix only to 2.15?

@julienfalque
Copy link
Member Author

julienfalque commented Nov 24, 2019 •
edited

Well, at the time I created the PR, 2.16 was not released yet so targeting branch master was fine.

The initial purpose of this PR was to refactor things to ease maintenance. While working on it, I noticed it made implementing parts of #4511 easy. When I noticed it also fixes #4605, the work on this PR was already finished. I only added the test cases, which passed without further work. I'm not sure fixing those bugs without leveraging this refactoring is easy. I'll give it a try.

Edit: actually tests from #4605 needed extra work, didn't remember :)

@julienfalque
Copy link
Member Author

Tests fixed in #4605.

@julienfalque julienfalque mentioned this pull request Feb 13, 2020
Closed
@julienfalque julienfalque changed the base branch from master to 2.16 April 5, 2020 11:45
@GrahamCampbell
Copy link
Contributor

Would something like this do:

image

@julienfalque
Copy link
Member Author

I rebased this PR to fix the conflict but did not apply any extra change. Let's merge this first and work on PHP 8 compatibility separately please.

@GrahamCampbell
Copy link
Contributor

The reason I suggest that change is because support for mixed was added a few days ago, so this PR should not now break it.

@GrahamCampbell
Copy link
Contributor

I think the php 8 tests now fail after this pr, but were passing before.

GrahamCampbell
GrahamCampbell suggested changes Nov 23, 2020
View reviewed changes
Copy link
Contributor

@GrahamCampbell GrahamCampbell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Broken unit tests on php 8

@julienfalque
Copy link
Member Author

@GrahamCampbell My bad, I didn't notice this was already supported on upstream branch, fixed as you suggested.

@GrahamCampbell
Copy link
Contributor

Needs rebasing again, against 2.17.

@julienfalque
Copy link
Member Author

Actually, if I remember correctly, all "new features" are marked @internal until 2.18 (should it be 3.0?) so it can still be merged into 2.16.

@GrahamCampbell
Copy link
Contributor

I think 2.17 is the right place for this PR.

@julienfalque
Copy link
Member Author

Why?

@GrahamCampbell
Copy link
Contributor

Because it adds new features, and 2.16 should only be getting bug fixes?

@julienfalque
Copy link
Member Author

The PR:

  • adds scalar_types option to phpdoc_to_param_type rule, but actually this is a revert of PhpdocToParamTypeFixer - remove not used option #5092 which can be considered a BC break
  • adds new APIs that are marked @internal for now
  • improves an existing fixer, which is something we use to merge in lower branches.

So IMO everything can be merged in 2.16. What do you think @SpacePossum @keradus @kubawerlos?

@keradus keradus changed the base branch from 2.16 to 2.17 December 17, 2020 13:41
GrahamCampbell
GrahamCampbell suggested changes Jan 19, 2021
View reviewed changes
Copy link
Contributor

@GrahamCampbell GrahamCampbell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have been testing this PR for a number of weeks, we no issues. Left just a couple of tiny comments.

src/DocBlock/Annotation.php Outdated Show resolved Hide resolved
src/DocBlock/Annotation.php Outdated Show resolved Hide resolved
@keradus keradus changed the base branch from 2.17 to 2.18 January 24, 2021 21:38
@coveralls
Copy link

coveralls commented Apr 30, 2021 •
edited

Coverage Status

Coverage decreased (-0.3%) to 91.563% when pulling 462260d on julienfalque:types-expression-class into 4961df4 on FriendsOfPHP:2.18.

keradus
keradus reviewed May 3, 2021
View reviewed changes
src/DocBlock/Annotation.php Outdated Show resolved Hide resolved
keradus
keradus reviewed May 3, 2021
View reviewed changes
src/DocBlock/Annotation.php Outdated Show resolved Hide resolved
keradus
keradus approved these changes May 3, 2021
View reviewed changes
Copy link
Member

@keradus keradus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crazy amount of work. kudos!

@keradus
Copy link
Member

keradus commented May 3, 2021

Thank you @julienfalque.

@keradus keradus merged commit db7e1f1 into PHP-CS-Fixer:2.18 May 3, 2021
@keradus keradus added this to the 2.18.7 milestone May 3, 2021
@julienfalque julienfalque deleted the types-expression-class branch May 3, 2021 18:14
keradus added a commit that referenced this pull request May 3, 2021
@keradus
feature #4592 PhpdocToPropertyTypeFixer - introduction (julienfalque)
5698474 
This PR was merged into the 2.19-dev branch.

Discussion
----------

PhpdocToPropertyTypeFixer - introduction

This PR includes #4591 and depends on #4593 which should both be merged first.

Commits
-------

0382be5 PhpdocToPropertyTypeFixer - introduction
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GrahamCampbell GrahamCampbell GrahamCampbell requested changes

@keradus keradus keradus approved these changes

Assignees
No one assigned
Labels
kind/bug kind/enhancement kind/refactoring
Projects
None yet
Milestone
2.18.7
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@julienfalque @kubawerlos @keradus @GrahamCampbell @coveralls