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

DOC: Add a coding style guideline. #1316

Merged
merged 1 commit into from Oct 20, 2017

Conversation

Projects
None yet
5 participants
@jhlegarreta
Contributor

jhlegarreta commented Aug 8, 2017

Add a coding style guideline mainly aimed at establishing some criteria
for the example and rst documentation files.

@jhlegarreta

This comment has been minimized.

Contributor

jhlegarreta commented Aug 8, 2017

@Garyfallidis @arokem @skoudoro @dmreagan Please review the PR. It is still a reduced style guide, to be completed as I learn DIPY or revise more code, we receive suggestions, etc.

@codecov-io

This comment has been minimized.

codecov-io commented Aug 9, 2017

Codecov Report

Merging #1316 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1316   +/-   ##
=======================================
  Coverage   87.03%   87.03%           
=======================================
  Files         228      228           
  Lines       29058    29058           
  Branches     3128     3128           
=======================================
  Hits        25292    25292           
  Misses       3059     3059           
  Partials      707      707

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9242c79...7c809e6. Read the comment docs.

@coveralls

This comment has been minimized.

coveralls commented Aug 9, 2017

Coverage Status

Coverage remained the same at 85.329% when pulling fa8fc5a on jhlegarreta:AddCodingStyleGuideline into 5595842 on nipy:master.

DIPY relies on a set of files that serve as an additional pool of information
for users and contributors:
- reStructuredText files (``*.rst``): the files located in the ``<./doc>``
folder structure contain informaton about theoretical backgrounds of DIPY,

This comment has been minimized.

@skoudoro

skoudoro Aug 9, 2017

Member

informaton -> information

@jhlegarreta jhlegarreta force-pushed the jhlegarreta:AddCodingStyleGuideline branch from fa8fc5a to 240e830 Aug 9, 2017

automatically when requesting to push to DIPY. However, as a contributor to
DIPY, you should try to ensure that your code, including its comments

This comment has been minimized.

@skoudoro

skoudoro Aug 9, 2017

Member

I think you can add that we follow the numpy coding style

This comment has been minimized.

@skoudoro

skoudoro Aug 9, 2017

Member

and you can cite the Numpy detailed example here

This comment has been minimized.

@jhlegarreta

jhlegarreta Aug 9, 2017

Contributor

The example refers to a C-function; I guess you meant example.py, right? It's already linked from the numpy coding style doc.

This comment has been minimized.

@jhlegarreta

jhlegarreta Aug 9, 2017

Contributor

@skoudoro thanks for the review and for having spot my slip-ups! See whether commit ca65ba8 addresses the comments.

@jhlegarreta jhlegarreta force-pushed the jhlegarreta:AddCodingStyleGuideline branch from 240e830 to ca65ba8 Aug 9, 2017

@jhlegarreta

This comment has been minimized.

Contributor

jhlegarreta commented Aug 9, 2017

BTW, should this be linked from or to CONTRIBUTING.md? Or else, should we integrate it into there? The numpy and PEP8 paragraphs overlap.

@arokem

This comment has been minimized.

Member

arokem commented Aug 9, 2017

I think best to move duplication from CONTRIBUTING into this guide, and then link from the guide to the documentation page.

@coveralls

This comment has been minimized.

coveralls commented Aug 9, 2017

Coverage Status

Coverage remained the same at 85.329% when pulling ca65ba8 on jhlegarreta:AddCodingStyleGuideline into 5595842 on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Aug 9, 2017

Coverage Status

Coverage remained the same at 85.329% when pulling ca65ba8 on jhlegarreta:AddCodingStyleGuideline into 5595842 on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented Aug 9, 2017

Coverage Status

Coverage remained the same at 85.329% when pulling ca65ba8 on jhlegarreta:AddCodingStyleGuideline into 5595842 on nipy:master.

@jhlegarreta jhlegarreta force-pushed the jhlegarreta:AddCodingStyleGuideline branch from ca65ba8 to 93f491a Aug 10, 2017

@jhlegarreta

This comment has been minimized.

Contributor

jhlegarreta commented Aug 10, 2017

@arokem Done. Please review the new patch set.

I've included a line width limit criterion for examples and .rst files; I don't know whether that's written in stone, but it's something I've observed across the files. Please, comment appropriately, as I'd like to know whether this has a particular (historic/Unix-like editor?) reason.

@coveralls

This comment has been minimized.

coveralls commented Aug 10, 2017

Coverage Status

Coverage remained the same at 85.329% when pulling 93f491a on jhlegarreta:AddCodingStyleGuideline into 5595842 on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Aug 10, 2017

Coverage Status

Coverage remained the same at 85.329% when pulling 93f491a on jhlegarreta:AddCodingStyleGuideline into 5595842 on nipy:master.

[software systems](https://pypi.python.org/pypi/pep8) that will check your code
for PEP8 compliance, and most text editors can be configured to check the
compliance of your code with PEP8.
Code contributions should be formatted according to the [DIPY Coding Style Guideline](./doc/devel/coding_style_guideline.rst).

This comment has been minimized.

@arokem

arokem Aug 10, 2017

Member

Eventually, once the style guide gets to the website, we should link to the website docs, instead?

This comment has been minimized.

@jhlegarreta

jhlegarreta Aug 11, 2017

Contributor

True. I guess we'll need to wait until it gets to the website to get the URL. I set a reminder for this.

Coding style
------------
DIPY follows the numpy_ coding style, described in their

This comment has been minimized.

@arokem

arokem Aug 10, 2017

Member

This is more about docstrings than about "coding"

This comment has been minimized.

@arokem

arokem Aug 10, 2017

Member

In particular, I would move this to the "documentation" section below

This comment has been minimized.

@jhlegarreta

jhlegarreta Aug 11, 2017

Contributor

Makes sense. Removed the sentence from the Coding style section, and reformatted the one in the Documentation section in c51e652.

in the documentation, but please make sure that changes that are introduced
render properly into the HTML format that is used for the DIPY website.
When documenting functions, modules, class or methods we follow the

This comment has been minimized.

@arokem

arokem Aug 10, 2017

Member

I see -- it's already here! No need for that sentence in the previous section, IMO.

This comment has been minimized.

@jhlegarreta

jhlegarreta Aug 11, 2017

Contributor

Addressed in c51e652.

@arokem

This comment has been minimized.

Member

arokem commented Aug 10, 2017

Great work here, @jhlegarreta! I had just a couple of small comments.

@jhlegarreta jhlegarreta force-pushed the jhlegarreta:AddCodingStyleGuideline branch from 93f491a to c51e652 Aug 11, 2017

@jhlegarreta

This comment has been minimized.

Contributor

jhlegarreta commented Aug 11, 2017

@arokem Thanks for the review.

OK, I identified the source for the maximum line width limitation: PEP8 and numpy docstring both establish a limitation. So removed the sentence I introduced in the last commit. Although they're different (72 vs. 75), I guess we don't have strong preferences.

Should we establish a hard criterion, please let me know (since the current files have generally been limited to 80...).

@coveralls

This comment has been minimized.

coveralls commented Aug 11, 2017

Coverage Status

Coverage decreased (-0.002%) to 85.327% when pulling c51e652 on jhlegarreta:AddCodingStyleGuideline into 5595842 on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Aug 11, 2017

Coverage Status

Coverage decreased (-0.002%) to 85.327% when pulling c51e652 on jhlegarreta:AddCodingStyleGuideline into 5595842 on nipy:master.

@jhlegarreta jhlegarreta force-pushed the jhlegarreta:AddCodingStyleGuideline branch from c51e652 to d698b85 Sep 7, 2017

@coveralls

This comment has been minimized.

coveralls commented Sep 7, 2017

Coverage Status

Coverage decreased (-0.002%) to 85.333% when pulling d698b85 on jhlegarreta:AddCodingStyleGuideline into e498e68 on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Sep 7, 2017

Coverage Status

Coverage decreased (-0.002%) to 85.333% when pulling d698b85 on jhlegarreta:AddCodingStyleGuideline into e498e68 on nipy:master.

@jhlegarreta

This comment has been minimized.

Contributor

jhlegarreta commented Sep 8, 2017

The failing build in Travis seems to be unrelated to the patch set. I ignore the meaning of the USE_PRE flag, but I've seen that other builds have the build marked as Allowed Failures (e.g. https://travis-ci.org/nipy/dipy/builds/214169250).

@jhlegarreta jhlegarreta force-pushed the jhlegarreta:AddCodingStyleGuideline branch from d698b85 to 846de18 Sep 20, 2017

Jon Haitz Legarreta Gorroño
DOC: Add a coding style guideline.
Add a coding style guideline mainly aimed at establishing some criteria
for the example and rst documentation files.

@jhlegarreta jhlegarreta force-pushed the jhlegarreta:AddCodingStyleGuideline branch from 846de18 to 7c809e6 Oct 19, 2017

@jhlegarreta

This comment has been minimized.

Contributor

jhlegarreta commented Oct 19, 2017

Rebased 7c809e6 onto master to see if the errors go away (although the branch simply modifies a Markdown file, and AFAIK there is not hook/check on .md files).

@skoudoro

This comment has been minimized.

Member

skoudoro commented Oct 20, 2017

Thanks for that @jhlegarreta. I think it is ready to go. @arokem is it ok for you?

I do not think that we need a hard criterion for the maximum line width limitation.

Concerning the paths, written as <./doc/devel>, its deserve its own new PR :-)

@arokem

This comment has been minimized.

Member

arokem commented Oct 20, 2017

Yep. Looks good. Merging!

@arokem arokem merged commit 163676e into nipy:master Oct 20, 2017

3 checks passed

codecov/patch Coverage not affected when comparing 9242c79...7c809e6
Details
codecov/project 87.03% remains the same compared to 9242c79
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jhlegarreta pushed a commit to jhlegarreta/dipy that referenced this pull request Mar 6, 2018

Jon Haitz Legarreta Gorroño
DOC: Link Coding Style Guide ref in CONTRIBUTING to the website.
Link the Coding Style Guideline reference in `CONTRIBUTING.md` to the
website docs once it got merged, as suggested by @arokem in PR nipy#1316.

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

Jon Haitz Legarreta Gorroño
DOC: Link Coding Style Guide ref in CONTRIBUTING to the website.
Link the Coding Style Guideline reference in `CONTRIBUTING.md` to the
website docs once it got merged, as suggested by @arokem in PR nipy#1316.

@jhlegarreta jhlegarreta deleted the jhlegarreta:AddCodingStyleGuideline branch Oct 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment