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

Keeping class references fully qualified after moving #2208

Merged
merged 3 commits into from
Apr 18, 2023

Conversation

mamazu
Copy link
Contributor

@mamazu mamazu commented Apr 15, 2023

If a class name was fully qualified before moving, then it should be fully qualified after moving as well.

Fixes #2115

@@ -34,6 +34,18 @@ public function replaceReferences(

foreach ($classRefList as $classRef) {
assert($classRef instanceof ClassReference);

// Check if the class name is fully qualified and replace it with a fully qualified version of the new name
$firstClassNameCharacter = $source->__toString()[$classRef->position()->start()];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that this is probably not the right way to check if a class name is fully qualified or not. Any suggestions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like you could add a method to QualifiedName: wasFullyQualified. ATM it seems it then has the contents ["", "Foo", "Bar"] otherwise you could check if it was fully qualified in the static constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should work but the ClassMover namespace has it's own version of a FullyQualified name. Should I try to also add it to that? Because this changes a lot of tests if I do.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes , class mover has it's own stuff...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added it in both versions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAIK ClassMover doesn't use the Name package. Can we revert those changes? A name doesn't normally know if it was fully qualfiied ,,,,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing.

{
return $this->updater->textEditsFor(SourceCodeBuilder::create()->namespace($qualifiedName->__toString())->build(), Code::fromString($source->__toString()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

note Code:: need probably be entirely replcaed by TextDocument, so this type of refatoring isnt really necessaary

@dantleech dantleech merged commit 5b0c410 into phpactor:master Apr 18, 2023
9 checks passed
@dantleech
Copy link
Collaborator

merci!

@mamazu mamazu deleted the phpactor_2115_fqn_after_move branch April 18, 2023 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when using class:move with FQCN
2 participants