[Docs] Update and rework CONTRIBUTING.md #3859

johnsaigle · 2018-07-31T17:35:52Z

This pull request modifies the CONTRIBUTING document.

Changes made:

Update the base branch practices
Add code formatting and branch label colours
Re-organize information flow
Skew tone toward on-boarding new devs

ridz1208 · 2018-08-01T13:02:20Z

@zaliqarosli @um4r12 @jesscall please read and see if you have comments

ridz1208 · 2018-08-01T15:15:48Z

CONTRIBUTING.md

  include a patch for existing projects to apply to get your changes of which
-  should be placed in the corresponding SQL/VERSIONNUMBER/ directory.
-* Include a test for any new module in the modules/MODULENAME/test/
+  should be placed in the corresponding `SQL/VERSIONNUMBER/` directory.


this is now SQL/New_patches/

and clean-up or optional patches in SQL/Cleanup_patches/

ridz1208 · 2018-08-01T15:17:59Z

CONTRIBUTING.md

+* Add your new automated tests to TravisCI in the `.travis.yml`. 
+* Make sure you run PHP codesniffer using the standards file in
+  `docs/LorisCS.xml` before sending any pull request, otherwise the automated tests will fail.
+* Try and make all changes backwards-compatible with existing installations. If you must change something


not necessarily non-backwards compatible only. anything that might affect the data and/or code of the project should be flagged (try to formulate that in good english)

ridz1208 · 2018-08-01T15:18:58Z

CONTRIBUTING.md


-* You can browse some of our public [Issues](https://github.com/aces/Loris/issues)
+* You can browse some of our public [Issues](https://github.com/aces/Loris/issues). Issues tagged with (https://via.placeholder.com/15/0e8a16/000000?text=+) **Beginner Friendly** are good ones to tackle if you are new to LORIS development.


missing the ![] after tagged with

Correct SQL patch directory information. Add formatting for label. Clarify use of Cavest label.

jesscall · 2018-08-01T16:58:03Z

CONTRIBUTING.md

+#### Bug Fixes
+   - Branch: `bugfix`
+   - Label: ![](https://via.placeholder.com/15/cc9966/000000?text=+) **[branch] bugfix**
+   - Content: Generally these changes do not require SQL scripts and are concise with the sole objective to correct on single problem in the code.


minor typo - should read 'one single problem'

jesscall · 2018-08-01T17:04:51Z

I think this is a nice initiative!
For the most part it is informative and provides all the links needed to get started.
I think you achieved a friendly / inviting tone.
👍

(Not sure what the etiquette is for documentation, is this considered 'Passed Manual Tests' ?)

johnsaigle · 2018-08-01T17:08:54Z

Thanks @jesscall! Giving the PR an approval is fine for Documentation. 👍

ridz1208

last request

ridz1208 · 2018-08-02T02:00:09Z

CONTRIBUTING.md

@@ -3,45 +3,55 @@
 We'd love to have you contribute to Loris. The first thing you should do
 before contributing is probably sign up for the [LORIS developers' mailing list](http://www.bic.mni.mcgill.ca/mailman/listinfo/loris-dev).

+Your next step before issuing a pull request is to review our [Coding Standards](https://github.com/aces/Loris/blob/minor/docs/CodingStandards). If you are doing front-end development you should also check out our [React guidelines](https://github.com/johnsaigle/Loris/blob/180631-Contributing/LORIS_react.README.md).


@johnsaigle I think github supports relative links and in this case you would want to link to the coding standards on your own branch, not minor

Oops accident

now pointing to previously merged markdownified file

chnages made

zaliqarosli

everything else looks good! 👍

zaliqarosli · 2018-08-02T19:16:12Z

CONTRIBUTING.md

@@ -3,45 +3,55 @@
 We'd love to have you contribute to Loris. The first thing you should do
 before contributing is probably sign up for the [LORIS developers' mailing list](http://www.bic.mni.mcgill.ca/mailman/listinfo/loris-dev).

+Your next step before issuing a pull request is to review our [Coding Standards](./docs/CodingStandards.md). If you are doing front-end development you should also check out our [React guidelines](https://github.com/johnsaigle/Loris/blob/180631-Contributing/LORIS_react.README.md).


the current CodingStandards file doesn't have .md extension so link doesn't work

How does the React guidelines link work once merged? Atm, it has your name in it and so, is off your fork

@zaliqarosli

the CodingStandards.md will work once merged dont worry about that

the react guidlines needs to pint to the loris one, that should be changed

zaliqarosli · 2018-08-02T19:16:18Z

CONTRIBUTING.md

+#### Minor Changes and Small Features
+   - Branch: `minor`
+   - Label: ![](https://via.placeholder.com/15/996633/000000?text=+) **[branch] minor**
+   - Content: Features affecting self-contained components such as modules. Additions to Libraries, API or modules that do not change and function signatures. 


is function a noun or a verb here? should it maybe be "..that do not change, and function signatures" so it's read the right way?

@johnsaigle all yours !!
I dont talk england very best

modules that do not change and function signatures. maybe ... do not change functionality or function signatures ...

I think it's just a typo for "do not change any function signatures"?

I just copied this from the Wiki -- agreed it could be reworded. Will do so this afternoon.

i think dave is right

PapillonMcGill · 2018-08-06T12:17:53Z

CONTRIBUTING.md

+## Some Things To Keep In Mind
+
+* If your changes require any table modifications:
+    1. Modify the `SQL/0000*.sql` file(s) with your changes. These patches are applied during the LORIS install.


I would add "review our [SQL Standard]https://github.com/aces/Loris/blob/minor/docs/SQLModelingStandard.md" as item 1

driusan · 2018-08-06T15:09:26Z

CONTRIBUTING.md

+
+#### Bug Fixes
+   - Branch: `bugfix`
+   - Label: ![](https://via.placeholder.com/15/cc9966/000000?text=+) **[branch] bugfix**


"https://via.placeholder.com/15/cc9966/000000?text=+"?

If you look at the rendered Markdown it displays a little colourful box the same colour as our labels.

I just copied this part from the Wiki cause it looks good. Not sure what this site it for.

I had added these to the wiki, thats the only way I found to put colourful boxes to match the label colours

um4r12 · 2018-08-06T17:31:04Z

CONTRIBUTING.md

-
-* You can browse some of our public [Issues](https://github.com/aces/Loris/issues)
+* Add your new automated tests to TravisCI in the `.travis.yml`. 
+* Make sure you run PHP codesniffer using the standards file in


should it be changed to incorporate the command to run phpcs? eg.

Make sure you run PHP codesniffer using the standards file in docs/LorisCS.xml by running vendor/bin/phpcs --standard=docs/LorisCS.xml <path_to_changed_files> before sending any pull request, otherwise the automated tests will fail.

or atleast a reference to

Loris/docs/CodingStandards

Line 21 in 12135e9

with the command `vendor/bin/phpcs --standard=docs/LorisCS.xml [file]` for any

johnsaigle · 2018-08-06T18:26:02Z

@zaliqarosli @PapillonMcGill @um4r12 CHanges have been addressed. Please re-review when you can.

zaliqarosli · 2018-08-06T18:35:34Z

CONTRIBUTING.md

@@ -39,8 +39,7 @@ For more information about making well-organized pull requests, please read our
 * Include a test for any new module in the `modules/MODULENAME/test/`
  directory. You can look at other modules for examples of how to write tests.
 * Add your new automated tests to TravisCI in the `.travis.yml`. 
-* Make sure you run PHP codesniffer using the standards file in
-  `docs/LorisCS.xml` before sending any pull request, otherwise the automated tests will fail.
+* Make sure you run PHP codesniffer using the standards file in docs/LorisCS.xml by running `vendor/bin/phpcs --standard=docs/LorisCS.xml <path_to_changed_files>` before sending any pull request, otherwise the automated tests will fail.


docs/LorisCS.xml should still have those quotes around it?

I don't think so because the quotes in this edit refer to a bash command to run so it would sort of make it less clear.

driusan

The content looks good, but:

Remove the unnecessary links to a third party service
GitHub is spelled wrong (it should have a capital H)
Line lengths are unreadably long. (piping paragraphs through the unix fmt tool can fix that.)

johnsaigle · 2018-08-14T16:38:33Z

@driusan I addressed 1. and 2. but the links render into nice coloured boxes on the front-end which makes for a very nice reading experience on the GitHub UI. I got them from @ridz1208 in the Wiki.

PapillonMcGill · 2018-08-14T18:13:47Z

CONTRIBUTING.md

+* If your changes require any table modifications:
+    1. Review our [SQL standard](./docs/SQLModelingStandard.md).
+    2. Modify the `SQL/0000*.sql` file(s) with your changes. These patches
+    are applied during the LORIS install.  3. Include a patch to be


and 4. should be at the same distance from beginning of line as 1. and 2. for rendering.

~~@PapillonMcGill This is actually how it's intended to appear. 1. and 2. are sub instructions underneath the first * as they relate specifically to table modifications.~~

PapillonMcGill · 2018-08-14T18:15:00Z

CONTRIBUTING.md

+* Make sure you run PHP codesniffer using the standards file in
+docs/LorisCS.xml by running `vendor/bin/phpcs --standard=docs/LorisCS.xml
+<path_to_changed_files>` before sending any pull request,
+otherwise the automated tests will fail.  * Try and make all changes


* (2) should be at the beginning of the line for rendering.

@johnsaigle This is the same rendering problem as higher.

cmadjar

@johnsaigle Looks great!! Just a few minor typo to correct.

cmadjar · 2018-08-27T18:18:20Z

CONTRIBUTING.md

+
+## Pull Request Title and Description
+
+To make it easier for reiewers to locate pull requests with wich they have


typo in reviewers. (missing v)

typo in which too. (missing h)

cmadjar · 2018-08-27T18:24:07Z

CONTRIBUTING.md

+mailing list](http://www.bic.mni.mcgill.ca/mailman/listinfo/loris-dev).
+
+Your next step before issuing a pull request is to review our
+[Coding Standards](./docs/CodingStandards.md). If you are doing


Coding standards link appears to be broken.

johnsaigle · 2018-08-27T18:53:55Z

@cmadjar Thanks! All fixed now

cmadjar

LGTM

PapillonMcGill · 2018-09-05T14:21:52Z

CONTRIBUTING.md

+## Some Things To Keep In Mind
+
+* If your changes require any table modifications:
+    1. Review our [SQL standard](./docs/SQLModelingStandard.md).


Should "SQL standard" be "SQL standards"

If it is the case, the file should also be renamed. Who created that file? Oh wait, I created that file :-(.

I think either one is fine. :)

driusan · 2018-09-11T16:50:15Z

CONTRIBUTING.md

-   - Branch: `major` - Label:
-   ![](https://via.placeholder.com/15/4d3319/000000?text=+) **[branch]
-   major** - Content: Any change modifying a function signature in a
+   - Branch: `major` - Label: **[branch major** 


missing a close bracket here

johnsaigle · 2018-09-12T16:36:04Z

@driusan Could you dismiss your old review if you're satisfied with the changes?

John Saigle added 2 commits July 31, 2018 13:27

Update CONTRIBUTING.md

554e935

More changes

da4b8fa

johnsaigle added the Documentation PR or issue introducing/requiring modifications to the code documentation (test plans, wikis, docs) label Jul 31, 2018

johnsaigle added this to the 20.1.0 milestone Jul 31, 2018

Add front-end guide links

010dc7f

ridz1208 requested changes Aug 1, 2018

View reviewed changes

John Saigle added 3 commits August 1, 2018 11:39

Rida's feedback

22a2d03

Correct SQL patch directory information. Add formatting for label. Clarify use of Cavest label.

Add colours and better format branch section

4002c66

Rewording

a161cee

jesscall reviewed Aug 1, 2018

View reviewed changes

Update CONTRIBUTING.md

358d524

jesscall previously approved these changes Aug 1, 2018

View reviewed changes

ridz1208 previously requested changes Aug 2, 2018

View reviewed changes

made link change

e25550c

now pointing to previously merged markdownified file

zaliqarosli requested changes Aug 2, 2018

View reviewed changes

Update CONTRIBUTING.md

079d772

PapillonMcGill suggested changes Aug 6, 2018

View reviewed changes

driusan reviewed Aug 6, 2018

View reviewed changes

um4r12 reviewed Aug 6, 2018

View reviewed changes

John Saigle added 3 commits August 6, 2018 14:20

Function signatures typo

186370d

Review SQL standard

c84b26a

phpcs details

d4582ed

um4r12 previously approved these changes Aug 6, 2018

View reviewed changes

zaliqarosli reviewed Aug 6, 2018

View reviewed changes

zaliqarosli previously approved these changes Aug 6, 2018

View reviewed changes

zaliqarosli added the Passed Manual Tests PR has undergone proper testing by at least one peer label Aug 13, 2018

driusan added the Meta PR does something that organizes, upgrades, or manages the functionality of the codebase label Aug 14, 2018

driusan requested changes Aug 14, 2018

View reviewed changes

johnsaigle added 2 commits August 14, 2018 12:36

Better line lengths

d26e28c

Github -> GitHub

1b9256e

johnsaigle dismissed stale reviews from PapillonMcGill, zaliqarosli, um4r12, and jesscall via 1b9256e August 14, 2018 16:37

PapillonMcGill suggested changes Aug 14, 2018

View reviewed changes

johnsaigle added 2 commits August 15, 2018 13:32

Fix bad rendering of list

a5c70d6

Actually fix rendering

3a9f466

PapillonMcGill previously approved these changes Aug 22, 2018

View reviewed changes

cmadjar requested changes Aug 27, 2018

View reviewed changes

Cecile's feedback

5c91bff

johnsaigle dismissed PapillonMcGill’s stale review via 5c91bff August 27, 2018 18:53

zaliqarosli previously approved these changes Aug 27, 2018

View reviewed changes

cmadjar previously approved these changes Aug 27, 2018

View reviewed changes

PapillonMcGill reviewed Sep 5, 2018

View reviewed changes

Remove fun coloured boxes :(

544834c

johnsaigle dismissed stale reviews from cmadjar and zaliqarosli via 544834c September 11, 2018 16:32

typos

64f0c72

driusan reviewed Sep 11, 2018

View reviewed changes

PapillonMcGill approved these changes Sep 12, 2018

View reviewed changes

driusan approved these changes Sep 17, 2018

View reviewed changes

driusan merged commit 03eea5c into aces:bugfix Sep 17, 2018


		* You can browse some of our public [Issues](https://github.com/aces/Loris/issues)
		* You can browse some of our public [Issues](https://github.com/aces/Loris/issues). Issues tagged with (https://via.placeholder.com/15/0e8a16/000000?text=+) Beginner Friendly are good ones to tackle if you are new to LORIS development.


		## Pull Request Title and Description

		To make it easier for reiewers to locate pull requests with wich they have

[Docs] Update and rework CONTRIBUTING.md #3859

[Docs] Update and rework CONTRIBUTING.md #3859

Conversation

johnsaigle commented Jul 31, 2018

ridz1208 commented Aug 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesscall commented Aug 1, 2018

johnsaigle commented Aug 1, 2018

ridz1208 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaliqarosli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnsaigle commented Aug 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

driusan left a comment

Choose a reason for hiding this comment

johnsaigle commented Aug 14, 2018

PapillonMcGill Aug 14, 2018 • edited

Choose a reason for hiding this comment

johnsaigle Aug 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmadjar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnsaigle commented Aug 27, 2018

cmadjar left a comment

Choose a reason for hiding this comment

PapillonMcGill Sep 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnsaigle commented Sep 12, 2018

PapillonMcGill Aug 14, 2018 •

edited

johnsaigle Aug 14, 2018 •

edited

PapillonMcGill Sep 5, 2018 •

edited