Skip to content

Conversation

@come-nc
Copy link
Contributor

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

Summary

And so it begins…

  1. Added rector in a vendor-bin
  2. Added a mostly-empty configuration
  3. Applied to apps folder

Long term goal is to apply rectors from https://github.com/nextcloud-libraries/rector to core code. We’ll need to be extra careful with folders like lib/public because they use some deprecated stuff on purpose I think.
So not sure if the best way forward is to add more rules or more folders.

Checklist

@come-nc come-nc added the 3. to review Waiting for reviews label Sep 19, 2024
@come-nc come-nc self-assigned this Sep 19, 2024
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.

Can you add a script like https://github.com/nextcloud/groupfolders/blob/532f734b9d729a3c29a7371faf0bb85a24e0b311/composer.json#L26? This makes it a lot easier to use rector.

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
Copy link
Member

Uhm is it really supposed to run on 50323 files? To me it looks like it includes way too much.

Only applying on apps for now, and only minimal type coverage level.

Signed-off-by: Côme Chilliet <[email protected]>
@come-nc come-nc force-pushed the feat/add-rector-config branch from ca417b5 to f9f95cf Compare September 20, 2024 15:51
@come-nc
Copy link
Contributor Author

come-nc commented Sep 20, 2024

Uhm is it really supposed to run on 50323 files? To me it looks like it includes way too much.

1877 for me

@provokateurin
Copy link
Member

Maybe because I have my cloned apps in apps/ as well, and then it also scans all the vendors of those and so on.
When ignoring everything that git ignores it should work though.

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.

Works, you only need to make the CI happy.

@come-nc come-nc merged commit 8927510 into master Sep 23, 2024
@come-nc come-nc deleted the feat/add-rector-config branch September 23, 2024 13:10
@skjnldsv skjnldsv mentioned this pull request Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants