GenerateAccessor handles both offset and names by camilledejoye · Pull Request #24 · phpactor/code-transform

camilledejoye · 2019-06-10T10:04:14Z

lib/Adapter/WorseReflection/Refactor/WorseGenerateAccessor.php

tests/Adapter/AdapterTestCase.php

dantleech · 2019-06-10T12:36:01Z

That's probably fine, we're testing the transformation. We can validate the method name in other tests.

On 10 June 2019 14:06:56 CEST, ely ***@***.***> wrote: elythyr commented on this pull request. > @@ -48,4 +48,16 @@ protected function sourceExpectedAndOffset($manifestPath) return [ $source, $expected, $offsetStart, $offsetEnd ]; } + + protected function sourceExpectedAndWordUnderCursor($manifestPath): array + { + list($source, $expected) = $this->sourceExpected($manifestPath); + + preg_match('/(\w+)<>(\w+)/', $source, $matches); + $word = $matches[1] . $matches[2]; + + $source = preg_replace('/<>/', '', $source); + + return [ $source, $expected, $word ]; We could keep it so we can still use the same data provider in the test. If we don't then I feel like we shouldn't use the `generateAccessorX.test` files, because it will enforce a implicit rule to name the tested property `method` and nothing else. -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #24 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

dantleech · 2019-06-10T12:41:53Z

I think this could be an exception to that law. The reflection API is a tree structure and is self-contained (and immutable).

…

On 10 June 2019 14:04:04 CEST, ely ***@***.***> wrote: elythyr commented on this pull request. > + } + + public function generateFromPropertyName(SourceCode $sourceCode, string $propertyName): SourceCode + { + $class = $this->class((string) $sourceCode); + $property = $class->properties()->get($propertyName); + + $info = SymbolContext::for(Symbol::fromTypeNameAndPosition( + Symbol::PROPERTY, + $property->name(), + $property->position() + ))->withContainerType(Type::class($class->name())) + ->withTypes($property->docblock()->vars()->types()) + ; + + return $this->generate($info, $sourceCode); I even wanted to gave it only a `ReflectionProperty` since it have all the needed information. For the namespace and class name `$property->class()->name()` and `$property->class()->name()->namespace()`. But to respect the law of demeter I propose to create: * `ReflectionClassLike::namespace()` * `ClassMemberReflection::namespace()` * `ClassMemberReflection::className()` -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #24 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

camilledejoye · 2019-06-10T12:54:37Z

That's probably fine, we're testing the transformation. We can validate the method name in other tests.

I think we didn't understand each over because I forgot to update the test so it uses the property name when calling the generate method.
For me the goal is not to test the property name but just to ensure that not matter how we name the property, the tests won't fail.

I think this could be an exception to that law. The reflection API is a tree structure and is self-contained (and immutable).

We can keep it self-contained and immutable, for instance with ReflectionClass:

    public function namespace(): string
    {
        return $this->name()->namespace();
    }

But I think this way it makes more sense.
It will allow to shorten the code and to have something logical like a property which provides the name of it's own class.

Still it's an early optimization, it could wait another PR.
Especially since there is some overlapping between some classes like ReflectionInterface and ReflectionClass in WR.
So we could handle this and in the same time provide a new abstract class or trait to centralize some parts of the code.

camilledejoye · 2019-06-10T13:02:56Z

Another idea while I think about it.
It might be interesting to refactor the formatName() method into it's own class, like GetterFormatter or something.
I would remove some dependencies from the WorseGenerateAccessor and allow for a more configurable option to the user.

But once again, maybe a too early optimization... I kind of often do that ><

EDIT:
One more thing, to deal with a property name we need the class now.
So right now we got an exception if the file contains more that one class.
It might be more in the spirit of Phpactor to also provide the class name of the property to be able to handle multiple class in the same file ?

Don't understand was there was an error anyway but...

dantleech · 2019-06-10T14:11:56Z

I think we didn't understand each over because I forgot to update the test so it uses the property name when calling the generate method.

Replied to comment in the code.

We can keep it self-contained and immutable, for instance with ReflectionClass:

I guess my question is: what value do we gain?

Positive:

it's shorter, and easier to mock.

Negative:

There are no 2 ways to do the same thing.
All implementations need to add another method.

Neutral:

We shouldn't mock WorseReflection anyway -- it's too complicated by nature.

It also depends on how you look at the role of the name / namespace. If you consider the name to be an FQN, then the namespace should be in the name object, if you think that the class is in a namespace AND it has a name, then it would make sense to have a separate namespace method.

It may well be the second way is more correct (possibly) but the first way is how it is currently modelled, so we should keep it like that, at least for now.
Note that BetterReflection returns the FQN from name -- as a string. Anyway, another discussion :)

camilledejoye · 2019-06-10T14:18:10Z

I agree with you.
Plus there is no rush, so I propose to let the things are they are for now and it will always be time to change it later is the need is stronger.

dantleech · 2019-06-10T14:19:06Z

It might be interesting to refactor the formatName() method into it's own class, like GetterFormatter or something.

Yeah, let's discuss in another issue -- there have also been requests for generating "setters" also (:scream: , but I guess can be valid for things like DTOs). But in general, all for extracting classes :+1:

It might be more in the spirit of Phpactor to also provide the class name of the property to be able to handle multiple class in the same file ?

What has been done in this situation before is to pass the cursor offset and let Phpactor determine the name of the class. I'm not sure where ... I'll try and look.

camilledejoye · 2019-06-10T14:28:23Z

Yeah, let's discuss in another issue -- there have also been requests for generating "setters" also (scream , but I guess can be valid for things like DTOs). But in general, all for extracting classes +1

Not a big fan of setters neither, even for DTOs. Most of the time I think we are better of with immutable objects in those cases so no need for setters.
I will let this one to some one else :)

Is there a todolist or idea box somewhere to register this kind of things for rainy days with nothing better to do ? :D

What has been done in this situation before is to pass the cursor offset and let Phpactor determine the name of the class

That's a possibility, but isn't it asking to Phpactor to do the job of the underlying plugin used by the editor ?
I just kept thinking about Phpactor as a binary, as a human I would like to tell Phpactor: "Generate the accesor for property in class ClassName in the stream SourceCode"

dantleech · 2019-06-10T14:32:46Z

Is there a todolist or idea box somewhere to register this kind of things for rainy days with nothing better to do ? :D

Just the issue list :)

Should had been in the previous commit but forgot. In order to bo able to test the retriving of the class from the cursor position I couldn't use the cursor to find out the name of the property.

camilledejoye · 2019-06-15T07:53:45Z

What has been done in this situation before is to pass the cursor offset and let Phpactor determine the name of the class. I'm not sure where ... I'll try and look.

I tried to do something like that.
If that's ok to you I think you are good to go.
Like last time I will update the corresponding PR for Phpactor with a composer update once this one will be merged.

lib/Adapter/WorseReflection/Refactor/WorseGenerateAccessor.php

lib/Domain/Refactor/GenerateAccessor.php

lib/Adapter/WorseReflection/Refactor/WorseGenerateAccessor.php

dantleech

Thanks again for your effort on this! Some CS comments relating to docblocks, but in general it looks good :)

dantleech · 2019-06-20T19:42:42Z

tests/Adapter/WorseReflection/Refactor/WorseGenerateAccessorTest.php

+            ],
+            'multiple-classes' => [
+                'generateAccessor6.test',
+                $propertyName,


why do we add $propertyName when it is always the same?

Would have slightly preferred to keep "constants" as a value in the test rather than repeating here (we don't vary in this test). But not important :)

I always try to keep tests configurable, not saying it's a good thing.
It makes my life terrible some time ! :D
But that's why, I just don't like the idea of coupling even... Early optimization, sill got some work to do on that ><

dantleech · 2019-06-20T19:45:19Z

Thanks @elythyr

camilledejoye · 2019-06-21T19:11:16Z

My pleasure, I will update the composer.lock for the PR on phpactor

GenerateAccessor handles both offset and names

3fb1b38

dantleech reviewed Jun 10, 2019

View reviewed changes

lib/Adapter/WorseReflection/Refactor/WorseGenerateAccessor.php Outdated Show resolved Hide resolved

dantleech reviewed Jun 10, 2019

View reviewed changes

lib/Adapter/WorseReflection/Refactor/WorseGenerateAccessor.php Outdated Show resolved Hide resolved

dantleech reviewed Jun 10, 2019

View reviewed changes

tests/Adapter/AdapterTestCase.php Outdated Show resolved Hide resolved

Removes offset handling from GenerateAccesor

d9633b6

Camille Dejoye added 3 commits June 10, 2019 15:09

A bit of cleanup and renaming

750a744

Fixes phpstan errors for InvalidArgumentException

e262697

Fixes phpstan error for properties->get() method

594e998

Don't understand was there was an error anyway but...

Camille Dejoye added 3 commits June 10, 2019 17:15

Removes regexp and use ExtractOffset::fromSource

2cdc4aa

Uses the cursor position to find out the class to work on

fc7036d

Removes the extracting word under the cursor

921cada

Should had been in the previous commit but forgot. In order to bo able to test the retriving of the class from the cursor position I couldn't use the cursor to find out the name of the property.

Adds missing use statement

891b9a3

dantleech reviewed Jun 17, 2019

View reviewed changes

lib/Adapter/WorseReflection/Refactor/WorseGenerateAccessor.php Show resolved Hide resolved

dantleech reviewed Jun 17, 2019

View reviewed changes

lib/Domain/Refactor/GenerateAccessor.php Show resolved Hide resolved

dantleech reviewed Jun 17, 2019

View reviewed changes

lib/Adapter/WorseReflection/Refactor/WorseGenerateAccessor.php Show resolved Hide resolved

dantleech reviewed Jun 17, 2019

View reviewed changes

Camille Dejoye added 2 commits June 19, 2019 19:41

Reviews corrections

beab497

forgot php-cs again

e37b4d3

dantleech reviewed Jun 20, 2019

View reviewed changes

dantleech merged commit 7ad69ff into phpactor:master Jun 20, 2019

camilledejoye deleted the generate-accessors-rework branch June 21, 2019 19:30

Conversation

camilledejoye commented Jun 10, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dantleech commented Jun 10, 2019 via email

Uh oh!

dantleech commented Jun 10, 2019 via email

Uh oh!

camilledejoye commented Jun 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

camilledejoye commented Jun 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dantleech commented Jun 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

camilledejoye commented Jun 10, 2019

Uh oh!

dantleech commented Jun 10, 2019

Uh oh!

camilledejoye commented Jun 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dantleech commented Jun 10, 2019

Uh oh!

camilledejoye commented Jun 15, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dantleech left a comment

Choose a reason for hiding this comment

Uh oh!

dantleech Jun 20, 2019

Choose a reason for hiding this comment

Uh oh!

dantleech Jun 20, 2019

Choose a reason for hiding this comment

Uh oh!

camilledejoye Jun 21, 2019

Choose a reason for hiding this comment

Uh oh!

dantleech commented Jun 20, 2019

Uh oh!

camilledejoye commented Jun 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

camilledejoye commented Jun 10, 2019 •

edited

Loading

camilledejoye commented Jun 10, 2019 •

edited

Loading

dantleech commented Jun 10, 2019 •

edited

Loading

camilledejoye commented Jun 10, 2019 •

edited

Loading