[#2783] - Coding-style fixes for plugins.
- Closed
- 18 Jan 2014
- Medium
- Build: staging
- # 2783
- Diff
- chrisdavenport:coding-style-fixes
- Success The Travis CI build passed Details
User tests: Successful: Unsuccessful:
This fixes the vast majority of coding-style issues in the /plugins directory. It's a bit of an experiment to assess how easy/difficult it would be to bring the entire CMS into compliance and how much time it would take to do it.
| Labels |
Added:
?
?
?
|
||
I believe it's generally better to have a lot PRs which deal with one codestyle issue each than having a big one which changes ~500 lines in 41 files.
Speaking for myself I would not merge this PR simply because I can't be sure I didn't miss a typo somewhere when reviewing. However if it would be an atomic PR dealing with only a few lines it would be easy to merge without any tests just by looking at the code.
Maybe someone else is more confident in his review skills and will merge it 
Agreed. I'm happy to break it up into multiple PRs (not sure how to do that in git but I'm sure I'll figure it out). One PR per file, or one per plugin, or one per code style issue type? Of course, in the end there will be exactly the same number of changes to review. ;-)
This was very much an experiment to assess how big a task achieving phpcs compliance would be. The answer is that it took between 2 and 3 hours to do it once I had phpcs set up and working. Submitting changes as multiple PRs would take a bit longer but probably not much.
One PR per file, or one per plugin, or one per code style issue type?
Only speaking for myself. I'd say one per code style issue. Then the reviewer would face a lot of the same type of changes, which should be easy to review. However it may be harder to do the PRs for that.
Maybe wait for @mbabker what he means.
I'd do it as one PR per extension myself for most everything but components. Considering the smaller extensions don't have a lot of files in them, it's not that bad to review 3 or 4 files in one pull. If we did it one fix per PR, there would be modules that have 40+ PRs to fix them.
Components, I'd say judge accordingly. If it ends up being one file in the PR, so be it.
Closing for now. Will resubmit as multiple PRs. Thanks guys.
| Status | New | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2014-01-18 17:44:13 |
Thank you Chris.
Best,
Matt Thomas
@betweenbrain
http://matt-thomas.me/
http://betweenbrain.com/
https://github.com/betweenbrain
Sent from mobile. Please pardon any typos or brevity.
On Jan 18, 2014 12:44 PM, "Chris Davenport" notifications@github.com
wrote:
—
Reply to this email directly or view it on GitHub#2783
.
I didn't get a chance to fully review every change, but this looks great. Thanks @chrisdavenport !