CONTRIBUTING.md

[ranpafin]

I think it could turn useful to have a CONTRIBUTING.md file. Nothing fancy just some basic tasks to run before submitting a PR and maybe the general guidelines.

More so, Paraunit docker container comes packed with all the tools required to follow these rules, particularly (but not only):

• phpcbf
• phpcs

It would be even better to provide a custom version of the of such tools, even a contributing.sh with multiple tasks inside. Something that a developer can run before submitting his contribution.

---

TO DO

• [x] Choose a coding standard (full PSR2)
• [x] Sync scrutinizer with the new coding standard
• [x] Add git pre-commit hook
  • [x] /src directory
  • [x] /tests directory (need to fix non camel-case method names)
• [x] Add a CONTRIBUTING.md

[Jean85]

That would be nice! Right now I rely on my IDE and on Scrutinizer to check those things. Basically, I use PSR code style, Symfony standard for service naming, and I prefer spacing present around the ! and . operators.

[ranpafin]

@Jean85 yep, i've looked into the .scrutinizer file and i think we have a couple of option to help people get along with the project current code-style. I think t

The first would be creating a custom ruleset.xml taking all we can from PSR2 that does not trigger a huge scale autofix from PHPCBF. Link that file to both the .scrutinizer and to a custom bash script (contributing.sh) something like:

phpcs -p --standard=/path/to/custom_ruleset.xml --colors src/ phpcbf --standard=/path/to/custom_ruleset.xml --colors src/

The other option will be to enforce the current .scrutinizer configured code-style by creating a custom ruleset with only a couples of rules (spacing present around the ! and . operators.).

I think we should follow the first approach, it may require a bit of work at the beginning but then we can have 3 tools working with a single .xml file.

I've made some digging with the standard PSR2 ruleset of phpcs ruleset.xml i think we can make it work (aka without triggering an all-around autofix) with only removing 3/4 rules.

[Jean85]

Good! What rules we need to remove? Are we violating PSR-2 in some way? I would prefer not to! I would likely ignore just the line-length warnings, even the PSR says that is a soft-limit.

[EDIT] Found, the spaces around the ! operator at the start of a control structure violates that. I think we can ignore that, for the sake of readability.

Do you think we can use a pre-commit hook to enforce this formatting? Also, I agree on the XML proposal. Can you work on it? Maybe in a branch here, without forking, so I can help.

[fntlnz]

If you are wondering about a CONTRIBUTING.md file I'd suggest you to look also at ISSUES/PR templates. Take a look at what docker/docker did there: https://github.com/docker/docker/tree/master/.github

[Jean85]

I see @fntlnz , but we don't have enough traffic to warrant a template like those, at least not for issues... What are you suggesting we should add?

[fntlnz]

I was just pointing out that some projects integrates the CONTRIBUTING.md file with these templates.

On Monday, 21 March 2016, Alessandro Lai notifications@github.com wrote:

I see @fntlnz https://github.com/fntlnz , but we don't have enough traffic to warrant a template like those, at least not for issues... What are you suggesting we should add?

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/facile-it/paraunit/issues/41#issuecomment-199483647

[ranpafin]

@Jean85 sure, i'll create a branch so we can all look into.

2203_contributing

It's still in heavy wip but if you look at these lines you can see i've commented out some rules. That part triggered a big autofix. I've also inceased the max line lenght to 180 as we violated the 120 in some cases, but as you pointed out it's really a pointless rule to enforce (here).

If you run the contributing.sh script file it should point out any violation so we can work iterativly on adding/editing the ruleset file.

[Jean85]

I've launched the script, and the main result is, apart from some minor fixes that I like, a bunch of ! operators that are no more spaced.

PSR-2 and Symfony follow the no-space convention, I'm unsure about how to proceed. Should we align Paraunit to this standard? Or should we stay with the spaces?

I'm not sure on how to enforce the spacing, in the latter case..

[ranpafin]

I'm more inclined to remove the spacing altogether and bring the project closer to the PSR-2 standard.

I think it should be the goal of this branch to set a definitive code style guide so, even if there will be some changes (like the one above) it could make sense to proceed.

[Jean85]

Agreed. Anyhow, using the PSR2 standard, PHPCS leaves alone the space AFTER the ! operator. I think we could leave it, to increase readability without going off-standard.

This should generate a bit of corrections, but it's better do it now and stick with some fixed and well shared rules as PSR are.

[Jean85]

Ok, I've pushed the proposed fixes. Next and last step should be a post-commit git hook to impose the standard.

[ranpafin]

Hook is ready, it will be created at the first "composer install" command.

We still need a CONTRIBUTING.md then i think we're good.

[Jean85]

Shouldn't we suppress the output to avoid spamming the console when committing?

[EDIT] Also, shouldn't we impose the standard in the tests folder too?

[ranpafin]

@Jean85 i've changed the logic a bit, rather than forcing an autofix now it will only run phpcs and leave the code review job to the developer.

Also if phpcs fails (exit code != 0) the commit will be blocked and the user will see the the reason in the output. It's an hard limit i'm not sure if it's the right way.

Run composer install again to get the updated version of the hook and let me know what you think.

NOTE: i've lowered the severity on warning so we ignore the line limit

---

[EDIT] Also, shouldn't we impose the standard in the tests folder too?

At the current stage we have a few errors for tests not being camelCase in their method name. We could change that and have it working for the test directory as well.

[Jean85]

The Git hooks can always be avoided with git commit --no-verify, so it's not an hard limit ;) I've seen your changes, LGTM. You would add a --force or --autofix option to the script BTW, to force the changes.

[Jean85]

Ok I took a stab at the .md file: https://github.com/facile-it/paraunit/blob/2203_contributing/CONTRIBUTING.md

Please review it!

[ranpafin]

LGTM, i've mentioned docker at the beginning with a link to the installation process.

[Jean85]

Good! :+1: Ok, I've fixed and included the tests in the check. I'll open a PR for this branch to discuss and review it further