Updating contributing and testing (closes #2445) #2482

djarecka · 2018-03-03T02:44:29Z

I've change Nipype testing section and slightly change Contributing.md.

I did include changes in doc from #2477, I will resolve conflict after merge.

About Nipype testing section:

I've tried to remove as much as I could. I was checking suggestions regarding settings env variables on my OSX and within Linux container, and if I didn't find that they are needed I just removed the section.
- I didn't know what to do with MATLABCMD suggestion for Debian. I can see that MATLABCMD is different in the Debian container (/opt/mcr/v92/toolbox/matlab), but would need some help to create a more general form of this suggestion.
I've decided to use pytest command as the way of testing (just to simplify, I haven't removed anything from Makefile)
in the Docker section I decided to use a very simple form of command to be as close as "non-docker" command as possible. If you think that it should be closer to the one from CircleCI please change it.

As always feel free to improve my English.

satra · 2018-03-05T15:48:02Z

doc/devel/testing_nipype.rst

@@ -75,69 +63,50 @@ where ``$pathtomatlabdir`` is the path to your matlab installation and
 ``$platform`` is the directory referring to x86 or x64 installations
 (typically ``glnxa64`` on 64-bit installations).

-Skip tests
+Skipped tests
 ~~~~~~~~~~


fix the heading lengths for restructured text.

satra · 2018-03-05T15:48:27Z

doc/devel/testing_nipype.rst

-
-For building a continer for running nipype in Python 3.6::
+If the project is tested both on your local OS and within a Docker container you might have to remove all 
+``__pycache__`` directory before changing between system.


effigies

Some grammar, wording and other suggestions.

effigies · 2018-03-07T01:14:29Z

CONTRIBUTING.md

@@ -4,7 +4,9 @@ Welcome to the Nipype repository! We're excited you're here and want to contribu

 These guidelines are designed to make it as easy as possible to get involved. If you have any questions that aren't discussed below, please let us know by opening an [issue][link_issues]!

-Before you start you'll need to set up a free [GitHub][link_github] account and sign in. Here are some [instructions][link_signupinstructions].
+Before you start you'll need to set up a free [GitHub][link_github] account and sign in, here are some [instructions][link_signupinstructions].


The comma you added doesn't really work. You can use a period as before, or switch to a semicolon (;) and keep the lowercase "here".

effigies · 2018-03-07T01:18:15Z

CONTRIBUTING.md

@@ -4,7 +4,9 @@ Welcome to the Nipype repository! We're excited you're here and want to contribu

 These guidelines are designed to make it as easy as possible to get involved. If you have any questions that aren't discussed below, please let us know by opening an [issue][link_issues]!

-Before you start you'll need to set up a free [GitHub][link_github] account and sign in. Here are some [instructions][link_signupinstructions].
+Before you start you'll need to set up a free [GitHub][link_github] account and sign in, here are some [instructions][link_signupinstructions].
+If you are not familiar with version control system and Git, 


Perhaps "version control systems such as git,".

effigies · 2018-03-07T01:21:33Z

CONTRIBUTING.md

-Before you start you'll need to set up a free [GitHub][link_github] account and sign in. Here are some [instructions][link_signupinstructions].
+Before you start you'll need to set up a free [GitHub][link_github] account and sign in, here are some [instructions][link_signupinstructions].
+If you are not familiar with version control system and Git, 
+you will find introduction and links to tutorials [here](http://www.reproducibleimaging.org/module-reproducible-basics/02-vcs/).


Linking with "here" as the text can usually be improved by picking a more descriptive word or phrase and centering the text around that. One option:

[introductions and tutorials](<URL>) may be found on [ReproducibleImaging.org](https://www.reproducibleimaging.org/).

effigies · 2018-03-07T01:22:29Z

CONTRIBUTING.md

@@ -47,30 +49,37 @@ This allows other members of the Nipype development team to confirm that you are

 **2. [Fork][link_fork] the [Nipype repository][link_nipype] to your profile.**

-This is now your own unique copy of Nipype.
+This is now your own unique copy of Nipype repository.


If you're going to add "repository", you'll need "the" before "Nipype".

effigies · 2018-03-07T01:23:38Z

CONTRIBUTING.md

+This will add your version of nipype to your local python environment and
+install dependencies needed for development.
+
+Make sure to [keep your fork up to date][link_updateupstreamwiki] with the original Nipype repository.


Maybe we can introduce/reinforce the "upstream" vocabulary by replacing original with "upstream".

I tried to not used work "upstream" on purpose, just to minimize number of new words (you don't need to have "upstream" to keep your master up to date). However the link is about "upstream", so I would just add an extra link that describes it:
"Make sure to keep your fork up to date with the original Nipype repository, one way of doing it is to configure a new remote upstream repository and [syncing your fork with the upstream][link_updateupstreamwiki].

That seems fine. Again, though, the comma is doing a bit too much work. I would make it a new sentence at "One way".

Also, you're switching tenses in this sentence with "doing", "to configure" and "syncing". As a suggestion (with some other slight rewording): "One way to do this is to configure a new remote named "upstream" and to sync your fork with the upstream repository."

effigies · 2018-03-07T01:33:04Z

doc/devel/testing_nipype.rst

-The base nipype image is built as follows::
+Nipype is tested inside Docker containers and users can use nipype images to test local versions. 
+First, install the `Docker Engine <https://docs.docker.com/engine/installation/>`_.
+Nipype has one base docker image called nipype/base:latest, that contains several useful tools 


nipype/nipype:base

effigies · 2018-03-07T01:33:31Z

doc/devel/testing_nipype.rst

-  docker build -t nipype/base:latest -f docker/base.Dockerfile .
+Users can pull the nipype image for Python 3.6 as follows::
+
+  docker pull nipype/base:py36


nipype/nipype:py36

effigies · 2018-03-07T01:34:02Z

doc/devel/testing_nipype.rst

-developer.
-
-For building a continer for running nipype in Python 3.6::
+If the project is tested both on your local OS and within a Docker container you might have to remove all 


Comma after "container".

effigies · 2018-03-07T01:34:27Z

doc/devel/testing_nipype.rst


-The last examples assume that the example data is downladed into ~/examples and
-the ~/scratch folder will be created if it does not exist previously.
+


No blank lines at the end of the file.

effigies · 2018-03-07T01:36:20Z

CONTRIBUTING.md


 Review and discussion on new code can begin well before the work is complete, and the more discussion the better!
-In the worst case scenario, if the development team decides to pursue a different path than you've outlined, they'll close the pull request. That's really not so bad! :smile:


I might retain this line, unless it's generally agreed that it's unwanted. I think there's some value in indicating that some changes may not be accepted, especially in the context of encouraging discussion and agreement prior to putting in a huge amount of effort on the code.

I removed it, since the idea that some (more or less abstract) team just closes the PR sounded to me (taking a perspective of person who is new to open source) rather harsh and discouraging ;-) but it's very subjective opinion...

what about "Review and discussion on new code can begin well before the work is complete, and the more discussion the better! The development team can prefer a different path than you've outlined, so it's better to discuss it and get approval at the early stage of your work."

Sounds great, but I would use "The development team may ..." rather than "The development team can...".

satra · 2018-03-07T15:10:42Z

@djarecka - in addition to @effigies comments, this will need to be merged with master.

djarecka · 2018-03-07T16:19:23Z

@effigies - Thanks a lot for your comments!

effigies · 2018-03-07T18:45:09Z

LGTM.

djarecka · 2018-03-07T19:10:37Z

@satra @effigies do you have any comments regarding MATLABCMD in testing_nipype. As I mentioned in the description of PR, I believe it's not correct (at least in a container), but not sure how to change it

satra · 2018-03-07T19:15:14Z

@djarecka - we should check if that is still true. we can reword it as the following:

On Debian systems with a local copy of MATLAB installed, set the following environment variable before running tests

also in terms of testing you can use this singularity image and point to matlab on openmind to check.

https://github.com/satra/om-images/blob/master/Singularity

that container can run matlab, would just need to add nipype to it.

djarecka added 3 commits March 2, 2018 21:15

updating testing doc and a few changes to contributing.md

595a70d

adding links

f5ed23c

changing a link about PR

9803db7

satra reviewed Mar 5, 2018

View reviewed changes

suggestions from Satra

7ea3525

effigies reviewed Mar 7, 2018

View reviewed changes

effigies added this to the 1.0.2 milestone Mar 7, 2018

djarecka added 2 commits March 7, 2018 13:17

suggestions from Chris M.

86ecf33

resolving a conflict

5dd3807

sty: add clarity for debian

201a2ef

satra merged commit 04b440a into nipy:master Mar 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating contributing and testing (closes #2445) #2482

Updating contributing and testing (closes #2445) #2482

djarecka commented Mar 3, 2018

satra Mar 5, 2018

satra Mar 5, 2018

effigies left a comment

effigies Mar 7, 2018

effigies Mar 7, 2018

effigies Mar 7, 2018

effigies Mar 7, 2018

effigies Mar 7, 2018

djarecka Mar 7, 2018

effigies Mar 7, 2018

effigies Mar 7, 2018

effigies Mar 7, 2018

effigies Mar 7, 2018

effigies Mar 7, 2018

effigies Mar 7, 2018

djarecka Mar 7, 2018

effigies Mar 7, 2018

satra commented Mar 7, 2018

djarecka commented Mar 7, 2018

effigies commented Mar 7, 2018

djarecka commented Mar 7, 2018

satra commented Mar 7, 2018


		The last examples assume that the example data is downladed into ~/examples and
		the ~/scratch folder will be created if it does not exist previously.


		Review and discussion on new code can begin well before the work is complete, and the more discussion the better!
		In the worst case scenario, if the development team decides to pursue a different path than you've outlined, they'll close the pull request. That's really not so bad! :smile:

Updating contributing and testing (closes #2445) #2482

Updating contributing and testing (closes #2445) #2482

Conversation

djarecka commented Mar 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies 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

satra commented Mar 7, 2018

djarecka commented Mar 7, 2018

effigies commented Mar 7, 2018

djarecka commented Mar 7, 2018

satra commented Mar 7, 2018