Update property generation for typed properties #14

camilledejoye · 2019-12-07T20:49:19Z

I admit I'm a bit tired right now and extremely hungry so I wont go in too much details.
The goal was to be able to generate typed properties.
In order to be able to keep the old generation and the new one depending on project I had to add a management of template by version of PHP.
I also used the occasion to fix one or two thing in the generation of properties.

I made a lot of commit and I try to rely detail each one of them to explain why and how as much as possible.

There is just one thing, a commit that move a blank line from the ClassLikeUpdater class to the Property.php.twig template create some "problems" in two situations.
I'm open to discussion about that, it would be nice to find a solution but I think we would have to choose the smaller of two issues :)

Add tests for PHP7.4 code generation

I add this rule because when you create a new class and define a trait before creating a constructor (for example). When you create the constructor and you complete the attribute from it (transformation action) then the properties were added before the trait which was always a pain to correct manually.

Append a blank line before the propertiess to add if they are added after something else than the class's opening brace or another property.

Before we made a decision that if a property has a type it will have a phpdoc block because it was the only way to declare the type. I don't like this because this logic depends on the templates and don't have its place here anymore. So I removed it and added the blank line directly in the template if there is a phpdoc block. Plus it fix the issue when inserting multiple properties for the first time (no property in the class yet), then even if the properties have a type there were no blank line between the properties. Note: There is two cases where the blank line is a problem, when: - the property is inserted after the class's open brace - the is inserted after something else than the class's open brace or another property

Append a blank line but only when the first thing in the class is a method declaration. Otherwise there is already a blank line before the method declaration.

It makes more sense and goes along with `nextMember` that was already used. We were already checking that `lastProperty` was an instance of `PropertyDeclaration` which proove that the name was not suited.

Reduce a bit the code and simply the loop. `reset()` return `false` if the array is empty, that's why I default it to `null`.

Moving the blank line before a property in the template have created a few issue, as mentionied in the commit in question. This commit updates the unit tests to take those "side effects" into account.

dantleech · 2019-12-08T11:29:12Z

lib/Adapter/TolerantParser/Updater/ClassLikeUpdater.php

@@ -54,41 +55,48 @@ protected function updateProperties(Edits $edits, ClassLikePrototype $classProto
            return;
        }

-        $lastProperty = $classMembers->openBrace;
+        $previousMember = $classMembers->openBrace;


can we add a test(s) for the these fixes?

or is this just presentation? if it's not a functional change then the test is not so important.

The name it's just presentation.
But the fact that the blank line before new properties was moved to the template it's functional.
Didn't I update the tests already ?

dantleech · 2019-12-08T11:31:52Z

lib/Util/TextFormat.php

@@ -18,6 +18,10 @@ public function indent(string $string, int $level = 0)
    {
        $lines = explode(PHP_EOL, $string);
        $lines = array_map(function ($line) use ($level) {


test is missing for this class, but it would be good to add

dantleech · 2019-12-08T11:33:53Z

tests/Adapter/UpdaterTestCase.php

@@ -925,6 +925,7 @@ public function crawl()
                <<<'EOT'
 class Aardvark
 {
+
    /**


hmm, actually this change is controversial for me and is not the SF coding standard or PSR-12, would rather have no space here.

I would love to too :)
The thing is that I trade a "bug" for another one.
Before the changes if you had

<?php class Foo { use SomeTrait; public function __construct($a) { } }

And you try to complete the constructor, the property would have been added above the trait declaration, which is not PSR-12 compliant neither.

The problem here is that we want to add a blank line only if there is a phpdoc block for the new property.
But the phpdoc part is totally up to the template, so the updater can't know when to add one or not.
And the templates can't know what is the preview member to decide if the blank line should be avoid or not.

hmm, only thing I can suggest is making it as consistent as possible, if we trade one bug for another then let's not add this change.

The problem in general with this library is that it explicitly couples to the presenatation - one solution might be to write a simple CS/whitespace fixer using the Tolerant Parser AST, which can operate on a range of lines (hmm 🤔 )

We could then care less about whitespace concerns when building the source code.

I think it's consistent in the way that it's only in two scenario and only if using phpdoc.
Plus it's 100% compliant and bug free for PHP 7.4.
So I would said that the trade is favorable, otherwise we go back with the previous problems in all versions.

And most important it only appends (if I remember correctly), when you have no properties yet.
So it's in a pretty specific and rare case.

The use of the parser would make it clean of course, if we can manage to check the number of black lines between a declaration member and a function member.
I'm not sure how though, will have to check.

hmm, ok. if the pros outweigh the cons, the let's go with it.

The user of the parser would ...

I'll try and have a look at this

dantleech · 2019-12-08T11:34:19Z

tests/Adapter/GeneratorTestCase.php

@@ -584,6 +587,7 @@ class Rabbits implements Animal
    const LEGS = 4;
    const SKIN = 'soft';

+


there should be one blank line between member "types"

That's the same issue than above.
I forgot to add tests for the 7.4 version, once done you will see that the problem exists only with the phpdoc.

dantleech · 2019-12-08T11:45:00Z

Thanks for this and the nice atomic commits:

✔️ Always add property after traits & constants
✔️OK Move the new line before property with phpdoc in the template
✔️ Initialize nextMember right away
✔️ Rename lastProperty into previousMember
✔️ Fix TextFormat::indent() to not indent empty lines
❌ Prepend a blank line when adding properties for the first time: looking at the adjusted tests, this isn't compatible with PSR-12, there should be no empty line between the opening brace and the first member declaration
❌ Append a blank line after the properties: there should be only one empty line between member "blocks"

In general we should be as standard as possible with the coding format.

⭐ Also I would prefer we use the version functions, e.g. phpversion versioncompare, this will allow us to use simple versions e.g. 7.1, 7.4 etc. This makes more sense when you consider that composer.json can also specify a PHP platform override as 7.1.1 f.e.

dantleech · 2019-12-08T12:04:05Z

👍 in additionto the above I think it would make sense to include the version-specific selection logic in this library rather than the extension (this is where the templates are, and it follows that this library should support them).

The twig-independent logic (for sorting filtering the template paths) could be placed in Domain. It can then be integrated in the TwigRenderer - which will create a Twig env. if one is not passed (note that in the extension we DO pass a Twig extension).

The nice thing is then that we can integrate, prove and test this feature in this library, with no concern for the extension.

camilledejoye · 2019-12-08T15:24:26Z

Append a blank line after the properties: there should be only one empty line between member "blocks"

I thought that was handled already, I will have to check.

Also I would prefer we use the version functions, e.g. phpversion versioncompare

I think it's harder to read than simply compare an integer, but whatever you prefer I don't mind.

in additionto the above I think it would make sense to include the version-specific selection logic in this library rather than the extension

In my mind the version could be used for other extensions as well, so I wanted to put it in the most global scope as possible.
But we can move it for now since it's used only there and remove it later if another extension has the need for it.

The twig-independent logic (for sorting filtering the template paths) could be placed in Domain.

I didn't like this big block of coded neither.

It can then be integrated in the TwigRenderer - which will create a Twig env. if one is not passed

I'm not sure about that, it feels a lot less reusable.
For me we use twig as a template generator but it's still the responsibility of the user (the one instantiate it the transformer here) to know how to handle it's templates.
I feel like we will be breaking SRP.

dantleech · 2019-12-08T15:28:15Z

I think it's harder to read than simply compare an integer, but whatever you prefer I don't mind.

Harder to read the code maybe -- but in the long term this is exposed to users, so clearer to see in the config php_version: 7.1 f.e. and the directory names would look more intutive to me (and comparing with other specifications of the PHP version, as with composer above).

camilledejoye · 2019-12-14T13:09:16Z

The CI is failing because it does not have the modification of worse-reflection, once phpactor/worse-reflection#65 will be merged we will need to restart it.

camilledejoye · 2019-12-14T15:11:58Z

Ok, I have:

upadted the name of the PR
remove two commits related to templates override
I think I took care of every review you made, but I let you make sure of that

So we should be ready to merge once all review comments are closed.
Then the CI of phpactor/code-transform#30 should be OK

dantleech · 2020-01-18T15:27:08Z

@elythyr the "style fixing" PR has been merged now (again) 🎉 so there should be no more problems (🤞 ) with whitespace etc. Can we the changes related to that from the PR and update the tests accordingly?

dantleech · 2020-01-19T15:44:15Z

I'll cherry pick the null stuff from this PR ... :)

dantleech · 2020-01-19T16:00:06Z

Nullable types is now supported here: #19 (cherry picked the tests from this PR)

Closing this now - as anything else (e.g. property ordering) in this PR will need to be in a new PR anyway). Thanks for this.

camilledejoye · 2020-02-05T12:55:10Z

Thanks for that, again I'm sorry about not being active for a while now.
New work, new home, etc. I need to figure out how to reoder my time.
I love this project, I used it daily and I really want to help it getting better & bigger :)

camilledejoye added 8 commits December 7, 2019 18:04

Prepend a blank line when adding properties for the first time

eb7b2e9

Append a blank line before the propertiess to add if they are added after something else than the class's opening brace or another property.

Append a blank line after the properties

ae64a77

Append a blank line but only when the first thing in the class is a method declaration. Otherwise there is already a blank line before the method declaration.

Rename lastProperty into previousMember

8ebfe3a

It makes more sense and goes along with `nextMember` that was already used. We were already checking that `lastProperty` was an instance of `PropertyDeclaration` which proove that the name was not suited.

Initialize nextMember right away

78bc9a2

Reduce a bit the code and simply the loop. `reset()` return `false` if the array is empty, that's why I default it to `null`.

Fix TextFormat::indent() to not indent empty lines

5e89e90

Update the unit tests

fe335d2

Moving the blank line before a property in the template have created a few issue, as mentionied in the commit in question. This commit updates the unit tests to take those "side effects" into account.

camilledejoye mentioned this pull request Dec 7, 2019

Handle templates by php version phpactor/phpactor#857

Closed

dantleech reviewed Dec 8, 2019

View reviewed changes

camilledejoye added 6 commits December 14, 2019 16:03

WorseBuilderFactory handles nullable properties & parameters

5b105e8

WorseBuilderFactory handles nullable method return types

60f8881

Property.php.twig handle nullable types

798eda4

UpdaterTestCase test adding a nullable typed property

7580d42

UpdaterTestCase test adding a nullable typed parameter

4d6785e

Add TextFormatTest

a668a23

camilledejoye force-pushed the php7.4-handle-property-type branch from 0c00147 to a668a23 Compare December 14, 2019 15:06

camilledejoye changed the title ~~Update property generation + override templates by PHP version~~ Update property generation for typed properties Dec 14, 2019

dantleech mentioned this pull request Jan 11, 2020

Release 14 by February 1st phpactor/phpactor#868

Closed

8 tasks

dantleech closed this Jan 19, 2020

dantleech mentioned this pull request Jan 19, 2020

Nullable types #19

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update property generation for typed properties #14

Update property generation for typed properties #14

camilledejoye commented Dec 7, 2019 •

edited

dantleech Dec 8, 2019 •

edited

dantleech Dec 8, 2019

camilledejoye Dec 8, 2019

dantleech Dec 8, 2019 •

edited

camilledejoye Dec 14, 2019

dantleech Dec 8, 2019

camilledejoye Dec 8, 2019

dantleech Dec 8, 2019 •

edited

camilledejoye Dec 8, 2019

dantleech Dec 8, 2019

dantleech Dec 8, 2019

camilledejoye Dec 8, 2019

dantleech commented Dec 8, 2019 •

edited

dantleech commented Dec 8, 2019 •

edited

camilledejoye commented Dec 8, 2019 •

edited

dantleech commented Dec 8, 2019 •

edited

camilledejoye commented Dec 14, 2019

camilledejoye commented Dec 14, 2019

dantleech commented Jan 18, 2020

dantleech commented Jan 19, 2020

dantleech commented Jan 19, 2020

camilledejoye commented Feb 5, 2020

		@@ -584,6 +587,7 @@ class Rabbits implements Animal
		const LEGS = 4;
		const SKIN = 'soft';

Update property generation for typed properties #14

Update property generation for typed properties #14

Conversation

camilledejoye commented Dec 7, 2019 • edited

dantleech Dec 8, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dantleech Dec 8, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dantleech Dec 8, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dantleech commented Dec 8, 2019 • edited

dantleech commented Dec 8, 2019 • edited

camilledejoye commented Dec 8, 2019 • edited

dantleech commented Dec 8, 2019 • edited

camilledejoye commented Dec 14, 2019

camilledejoye commented Dec 14, 2019

dantleech commented Jan 18, 2020

dantleech commented Jan 19, 2020

dantleech commented Jan 19, 2020

camilledejoye commented Feb 5, 2020

camilledejoye commented Dec 7, 2019 •

edited

dantleech Dec 8, 2019 •

edited

dantleech Dec 8, 2019 •

edited

dantleech Dec 8, 2019 •

edited

dantleech commented Dec 8, 2019 •

edited

dantleech commented Dec 8, 2019 •

edited

camilledejoye commented Dec 8, 2019 •

edited

dantleech commented Dec 8, 2019 •

edited