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 rector config #48223

Merged
merged 4 commits into from
Sep 23, 2024
Merged

Add rector config #48223

merged 4 commits into from
Sep 23, 2024

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.

rector.php Outdated Show resolved Hide resolved
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.

rector.php Outdated Show resolved Hide resolved
@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
175 checks passed
@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.

3 participants