-
Notifications
You must be signed in to change notification settings - Fork 10
Add Rector to project #31
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
Conversation
demiankatz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @sambhavp96! I see that the build is failing here because PHP-CodeSniffer is complaining about the config file. I think we should also integrate this into Phing and GitHub Actions workflows. See https://github.com/vufind-org/vufindharvest/pull/36/files for a model that I think you should be able to imitate here to make further progress.
demiankatz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @sambhavp96, the build is now passing, but Rector is not yet running as part of continuous integration.
The bare minimum way of doing this would be to add the rector-console task to the list of phing tasks on line 21 of ci.yaml.
If we want to better align this with other projects, it would also be worthwhile to parallel the changes in vufind-org/vufindharvest#34 and vufind-org/vufindharvest#39.
Maybe the best approach would be to do the simple integration here, and then follow up with further tool updates and configuration in separate PRs after this gets merged. What do you think?
Yes, I think that's a good approach. Let me integrate it here, then I can create another PR. |
demiankatz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The build failed here because Rector has not been added to composer.json yet. Hopefully if that gets added, it will start working!
Also, I notice that codeQualityLevel is set to 18, but now that we've raised it to 22 on the main project, maybe we should make an equivalent change here (especially if it doesn't have code impacts in this project).
demiankatz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @sambhavp96!
No description provided.