-
Notifications
You must be signed in to change notification settings - Fork 18
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
Types are not set for parameters or return values in the WordPress plugin #625
Comments
We could run https://github.com/rectorphp/rector to automate a lot of what you're suggesting |
@tobeycodes Is this a one-time event to refactor the codebase or can we use rector on a continual basis? Do you think the codebase is large enough to warrant an automated solution versus going through the plugin file by file? |
Both. You can run it once or part of CI pipelines. In my opinion the size of a codebase should not be a factor in deciding whether to use tooling like this. I think there should still be a file by file review but automating things will catch things now we miss and keep us up to date in the future too. |
@claytoncollie You're definitely right that the plugin could get a bit more love. I'd love to see changes on that front too, feel free to submit a PR! I don't have experience with phpstan but I'm all for automated process to improve the codebase. |
@nicholasio I created four pull requests to address this issue. I think we can add a fifth PR in the future to introduce PHPStan but, for now, I would like to get your eyes on these changes and hear your feedback. I am not as familiar with this platform as you are so I will need guidance on how to test each of the changes. PRs 1,2,3 are focused on PHPCS with automatic and manual fixes. They are all chained so that you are not looking at one giant PR. The fourth PR is to introduce Rector and enforce those coding standards. @tobeycodes please have a look at the config of that tool to see if it matches up with how you are using it. |
### Description of the Change Automatic linting PR to enforce short array syntax. Require #689 Part 3 for addressing #625 ### Changelog Entry > Changed - Enforce short array syntax ### Credits Props @claytoncollie
### Description of the Change Requires #688 These are the manual fixes for the WP plugin. Part 2 for addressing #625 ### Changelog Entry > Changed - Address PHP coding standards manual fixes ### Credits Props @claytoncollie
Is your enhancement related to a problem? Please describe.
As I was looking through the codebase, I was impressed with how much time and effort was put into defining all of the parameter and return types in JavaScript. The work is very organized. Bravo! But then when I looked at the WordPress plugin, I thought there could be a bit more effort put into this codebase to bring it up to the same standard. Most of the time we are defining the types in a docblock which is good but should be taken a step further and codified as well such as
function get_something( string $param ): string { return 'something'; }
.I think a good first step is to all parameter and return types to each method and function in the WordPress plugin ( including
void
for functions that do not return a value) and then, as a second step, introduce PHPstan and a GitHub action to help future contributors adhere to the standard. We have an open pull request in the wp-scaffold repo that would be easy to copy from to introduce this functionality.I am happy to make a pull request for one or both of these ideas.
Designs
No response
Describe alternatives you've considered
No response
Code of Conduct
The text was updated successfully, but these errors were encountered: