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.

Sign up for GitHub

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

Auto import Nextcloud classes in rector runs #48371

Merged
merged 4 commits into from
Oct 15, 2024
Merged

Auto import Nextcloud classes in rector runs #48371

merged 4 commits into from
Oct 15, 2024

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Sep 26, 2024

Summary

When using rector, fully qualified names are often outputted by the rules, like \OCP\Server or \OCP\Http\Client\IClientService. To avoid that, one can use withImportNames() to tell rector to import names with use statements, but then it will import all names.

This is not always desirable, for instance \LDAP\Connection in user_ldap would be confusing because there is an \OCA\User_LDAP\Connection class as well.

So I added a custom "SkipVoter" which skips auto-import for all classes outside of OC/OCA/OCP.

It may still be a bit confusing for common names like IManager, but I think we can live with that? Or I can add other exceptions if needed.
Note that rector is smart enough to not import two classes with the same name, but it will randomly import one of them and keep the fully qualified name for the other one.

I’m doing that so that when we apply Nextcloud rules set later we do not get fully qualified names everywhere but nice use statements instead.

Checklist

@come-nc come-nc added the 3. to review Waiting for reviews label Sep 26, 2024
@come-nc come-nc added this to the Nextcloud 31 milestone Sep 26, 2024
@come-nc come-nc self-assigned this Sep 26, 2024
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 26, 2024
View reviewed changes
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Psalm found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

provokateurin
provokateurin reviewed Sep 26, 2024
View reviewed changes
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

I'm not sure if I like it. In quite a few places it is nice to use the fqn to avoid mistakes.
Then there is also the case when two classes have the same name where I would actually prefer that both use the fqn and not one fqn and one import.

build/rector.php

$nextcloudDir = dirname(__DIR__);

class NextcloudNamespaceSkipVoter implements ClassNameImportSkipVoterInterface {
Copy link
Member

Choose a reason for hiding this comment

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

This should go into nextcloud/rector then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could, yeah.
The thing is we cannot make it configurable, because it’s added as a service by class name only.
But if it stays general like this we can put in nextcloud/rector, yes. Also we can make it extendable with a protected property to override if needed.

@come-nc
Copy link
Contributor Author

come-nc commented Sep 26, 2024

In quite a few places it is nice to use the fqn to avoid mistakes.

Do you have examples in mind so that we can look if there is a pattern we could apply?

Then there is also the case when two classes have the same name where I would actually prefer that both use the fqn and not one fqn and one import.

That is quite hard to implement I think. The SkipVoter which avoid use conflict is based on the already imported use list: https://github.com/rectorphp/rector/blob/main/rules/CodingStyle/ClassNameImport/ClassNameImportSkipVoter/UsesClassNameImportSkipVoter.php
Doing the same thing without even adding the first use would require collecting all shortnames from the file before deciding whether to import them or not.

@provokateurin
Copy link
Member

Could we maybe limit this to a few classes like \OCP\Server and \OCP\Util for now? I don't know how many other cases we have where this happens a lot.

@come-nc
Copy link
Contributor Author

come-nc commented Sep 26, 2024

Could we maybe limit this to a few classes like \OCP\Server and \OCP\Util for now? I don't know how many other cases we have where this happens a lot.

My problem is this rule set: https://github.com/nextcloud-libraries/rector/blob/8061985c44798bac1657ba5bc2d6efa5b10e9089/config/nextcloud-25/nextcloud-25-deprecations.php#L17
Without withImportNames it will use fully qualified name for all Server::get() calls and that’s ugly.
I can try again a solution like https://github.com/nextcloud-libraries/rector/pull/11/files to instead make our rules add stuff to the use statements independantly of withImportNames. I failed before but I have a better understanding of rector internals now.

@come-nc
Copy link
Contributor Author

come-nc commented Oct 8, 2024

So, the approach in https://github.com/nextcloud-libraries/rector/pull/11/files failed.

@provokateurin What do you think about using the strategy in this PR (auto import from OC/OCA/OCP), but with a manually maitained list of exempted class names? (The list would be something like Plugin,Manager,IManager,Exception).
Can I try that and see what the result looks like on our apps?

@provokateurin
Copy link
Member

Sounds good to me, excluding common duplicate names 👍

@come-nc
Copy link
Contributor Author

come-nc commented Oct 10, 2024

Sounds good to me, excluding common duplicate names 👍

Can you check at the current diff if the exclusion list and the result makes sense to you?

provokateurin
provokateurin requested changes Oct 10, 2024
View reviewed changes
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

Looks good.
Maybe Node, File and Folder should be excluded as well since they also exist in \OCA\DAV\Connector\Sabre but this mostly affects the dav app so I'd leave that decision to @ChristophWurst and the likes.

build/rector.php Show resolved Hide resolved
provokateurin
provokateurin approved these changes Oct 14, 2024
View reviewed changes
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

Nice 👍

ChristophWurst
ChristophWurst approved these changes Oct 14, 2024
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Clicked into a bunch of random files and can't spot any issues with the imports

:shipit:

@come-nc
chore(rector): Add rule to import Nextcloud classes in all files
2815344 
We skip all files outside of OC/OCA/OCP from "use" auto-import because
 it’s not always desirable to import classes when they have generic
 names, so it should be the developer choice.

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
feat(rector): Skip classes with a name too common from auto use state…
4ff9b3e 
…ment

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
chore(apps): Apply new rector configuration to autouse classes
1580c86 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
chore: Update baseline after rector refactoring
be80943 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc

This comment was marked as outdated.

Sign in to view
@come-nc come-nc merged commit dc92218 into master Oct 15, 2024
179 checks passed
@come-nc come-nc deleted the fix/rector-use-statements branch October 15, 2024 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@st3iny st3iny st3iny approved these changes

@provokateurin provokateurin provokateurin approved these changes

@miaulalala miaulalala Awaiting requested review from miaulalala miaulalala is a code owner

@nickvergessen nickvergessen Awaiting requested review from nickvergessen nickvergessen is a code owner

@tcitworld tcitworld Awaiting requested review from tcitworld

Assignees

@come-nc come-nc

Labels
3. to review Waiting for reviews
Projects
None yet
Milestone
Nextcloud 31
Development

Successfully merging this pull request may close these issues.
4 participants
@come-nc @provokateurin @ChristophWurst @st3iny