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

Add documentation explanation for Auto Import #111

Closed
wants to merge 1 commit into from

Conversation

samsonasik
Copy link
Member

Q A
Documentation yes

Description

@froschdesign this is copied from the website https://getrector.org/documentation/import-names

Fixes #69

@@ -14,7 +14,15 @@ return static function (RectorConfig $rectorConfig): void {
};
```

If you want to make renamed class type hint auto import enabled, you may use `SetList::LAMINAS_SERVICEMANGER_40_AUTO_IMPORT` set list, so the `rector.php` config will be as follow:
Rector works with all class names as fully qualified by default.In the most projects, that's not a desired behavior, because short version with use statement is easier to read.
Copy link
Member

Choose a reason for hiding this comment

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

…is easier to read.

This should be avoid because it is easier for you or me does not mean that it is easier for everyone else.

Suggested change
Rector works with all class names as fully qualified by default.In the most projects, that's not a desired behavior, because short version with use statement is easier to read.
Rector works with fully qualified class names by default.
But most projects and coding standards use imported class names instead, like [laminas-coding-standard](https://docs.laminas.dev/laminas-coding-standard/).

If you want to make renamed class type hint auto import enabled, you may use `SetList::LAMINAS_SERVICEMANGER_40_AUTO_IMPORT` set list, so the `rector.php` config will be as follow:
Rector works with all class names as fully qualified by default.In the most projects, that's not a desired behavior, because short version with use statement is easier to read.

To import FQCN, configure rector.php with:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
To import FQCN, configure rector.php with:
To import fully qualified class name (FQCN), configure rector.php with:

$rectorConfig->importNames();
```

If you want to make renamed class type hint to use short name with import its fully qualified ot use statement, you may use `SetList::LAMINAS_SERVICEMANGER_40_AUTO_IMPORT` set list that utilize `$rectorConfig->importNames()` on it, so the `rector.php` config will be as follow:
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, I don't understand the sentence or what the statement is. I will work out another proposal.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I can close this PR for now so we can use yours :)

Copy link
Member

Choose a reason for hiding this comment

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

…so we can use yours…

Which one? 🤷‍♂️

Copy link
Member Author

Choose a reason for hiding this comment

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

When you have time to propose it, I believe you have better wording for it 👍

Copy link
Member

Choose a reason for hiding this comment

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

Why did you delete the branch? All you had to do was explain what you wanted to say, then we would have found a suitable wording.

Copy link
Member Author

Choose a reason for hiding this comment

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

You said you will work out another proposal, so I guess you had better wording already in mind :)

Copy link
Member

Choose a reason for hiding this comment

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

Help me out, otherwise I can't help you!

What is happening? Why is SetList::LAMINAS_SERVICEMANGER_40_AUTO_IMPORT needed or better than $rectorConfig->importNames() or should both be used?

Copy link
Member Author

Choose a reason for hiding this comment

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

The SetList::LAMINAS_SERVICEMANGER_40_AUTO_IMPORT include the base LAMINAS_SERVICEMANGER_40 which auto import enabled.

There is no better or worst, just a shortcut (1 statement vs multi).

user can directly use $rectorConfig->importNames(); after if they want.

Copy link
Member

Choose a reason for hiding this comment

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

What is the recommended way? I think $rectorConfig->importNames();, as documented in the official documentation: https://getrector.com/documentation/import-names

So SetList::LAMINAS_SERVICEMANGER_40_AUTO_IMPORT is only nice to have and because it corresponds to the coding standard of Laminas. Right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, $rectorConfig->importNames() probably better, but since Rector still version 0.*, anything can changed about the name.

The SetList::LAMINAS_SERVICEMANGER_40_AUTO_IMPORT is a shortcut in case user don't want to type multi statements in config, also safer as the config call already included.

@samsonasik samsonasik closed this Feb 8, 2023
@samsonasik samsonasik deleted the add-autoimport-doc branch February 8, 2023 04:37
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.

Explanation for "auto import" is missing
2 participants