Skip to content
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: Fix reStructuredText formatting issues in coding style guideline. #1421

Merged
merged 1 commit into from Feb 13, 2018

Conversation

jhlegarreta
Copy link
Contributor

Fix RST markup issues in the coding_style_guideline.rst file.

Fixes #1390.

@jhlegarreta
Copy link
Contributor Author

@jchoude Thanks for reporting the issue !

Rather than checking the GitHub file, it looks like checking the dipy website version gives a more intuitive idea of what's going on.

The mission file seems to have issues as well, but it's rendered correctly.

Please, correct me if I'm wrong, and if both versions should be rendered correctly.

@codecov-io
Copy link

codecov-io commented Feb 10, 2018

Codecov Report

Merging #1421 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1421      +/-   ##
==========================================
- Coverage   87.39%   87.38%   -0.01%     
==========================================
  Files         237      237              
  Lines       30069    30069              
  Branches     3232     3232              
==========================================
- Hits        26278    26277       -1     
  Misses       3043     3043              
- Partials      748      749       +1
Impacted Files Coverage Δ
dipy/core/graph.py 73.8% <0%> (-1.2%) ⬇️

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 666d783...e332c38. Read the comment docs.

Copy link
Member

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this @jhlegarreta !

Can I request some additional change?

  • Can you add release 0.12 and 0.13 on this page
  • Can you replace HBM 2015by ÒHBM 2015 on this page

object called ``GradientTable`` which holds all the acquisition specific
parameters, e.g. b-values, b-vectors, timings and others.*
* Cite the relevant papers. Use the *[NameYear]* convention for
cross-referencing them, such as in [Garyfaillidis2014]_, and put them
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Garyfallidisinstead of Garyfaillidis

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks ! I inspected this visually yesterday without being able to tell why it was not being linked 😬

DIPY object, etc.).
* When referring to relative paths, use the backquote inline markup
convention, such as in ``doc/devel``. Do not add the
greater-than/less-than signs to enclose the path.


"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need this quotes? I think you can remove them because they look strange as you can see on this page

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they make easier reading the text, standing out what is a code snippet or any other construct that belong to the structure of the toolkit. The reason why they are double inverted quotes instead of simple inverted quotes is due to the Sphinx syntax in the docstrings (if I remember well).

A question relative to the way dirs had to be described was raised in a comment #1314 .

Copy link
Member

@skoudoro skoudoro Feb 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are not talking about the same line. I am talking about the double quotes here, line 113

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, now I got it. Sorry. My bad.

@@ -117,4 +123,5 @@ References
<http://journal.frontiersin.org/Journal/10.3389/fninf.2014.00008/abstract>`_
Frontiers in Neuroinformatics, vol.8, no.8.

.. include:: ../links_names.inc
"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment as above

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above. The double quotes are not necessary here.

@skoudoro
Copy link
Member

Concerning the rendering, I wonder why GitHub does not take in consideration link_names.inc

@jhlegarreta jhlegarreta force-pushed the FixCodingStyleGuidelineFormatting branch from bbd4260 to 6ec74bc Compare February 11, 2018 10:41
@jhlegarreta
Copy link
Contributor Author

Yes, I also realized that GitHub does not link the cross-refs in link_names.inc but have not investigated 😟

@skoudoro
Copy link
Member

It seems that github don't allow the include directive for security reason (look here).

@jhlegarreta jhlegarreta force-pushed the FixCodingStyleGuidelineFormatting branch from 6ec74bc to 244a049 Compare February 12, 2018 18:11
@jhlegarreta
Copy link
Contributor Author

OK. Fixed the quotes. Thanks for the explanation on the include directive issue @skoudoro

@skoudoro
Copy link
Member

  • Can you add release 0.12 and 0.13 on this page
  • Can you replace HBM 2015by ÒHBM 2015 on this page

It will be nice if you can fix these additional issues on this PR. Thank you !

@jhlegarreta jhlegarreta force-pushed the FixCodingStyleGuidelineFormatting branch from 244a049 to e332c38 Compare February 12, 2018 18:45
@jhlegarreta
Copy link
Contributor Author

Sorry Serge. I missed that in your first comment. Done.

Fix RST markup issues in the `coding_style_guideline.rst` file.

Fixes dipy#1390.
@skoudoro
Copy link
Member

Thank you @jhlegarreta, it looks better now after test.

merging !

@skoudoro skoudoro merged commit 7600480 into dipy:master Feb 13, 2018
@jhlegarreta jhlegarreta deleted the FixCodingStyleGuidelineFormatting branch February 14, 2018 14:09
ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018
…Formatting

DOC: Fix reStructuredText formatting issues in coding style guideline.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants