Skip to content

Conversation

@marcobarilari
Copy link
Collaborator

No description provided.

@pep8speaks
Copy link

pep8speaks commented Oct 25, 2020

Hello @marcobarilari! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-10-31 19:23:08 UTC

@marcobarilari marcobarilari marked this pull request as draft October 25, 2020 14:09
@codecov
Copy link

codecov bot commented Oct 25, 2020

Codecov Report

Merging #138 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev     #138   +/-   ##
=======================================
  Coverage   21.63%   21.63%           
=======================================
  Files          55       55           
  Lines         795      795           
=======================================
  Hits          172      172           
  Misses        623      623           
Flag Coverage Δ
unittests 21.63% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


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 0b51544...19fd82f. Read the comment docs.

@marcobarilari
Copy link
Collaborator Author

Hey @Remi-Gau, I followed the instructions in the spm pipeline repo and something happened but I have no idea about I am doing and if it is working. Sphinx is still very unclear to me

@Remi-Gau
Copy link
Contributor

Are you saying you can't figure out the riddles that Sphinx is throwing you?
I guess they have chosen the name correctly.

Will have a look right away.

@@ -0,0 +1,26 @@
# .readthedocs.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why you have several .readthedocs.yml files in the docs folder.

As long as you have one in the root folder, you are good to go.

Also the read the docs part comes later and is only to use the website "read the docs" to display the documentation on line. So this is not purely "sphinx" related.

For example "read the docs" can also render docs written with "mkdocs".

# add these directories to sys.path here. If the directory is relative to the
# documentation root, use os.path.abspath to make it absolute, like shown here.
#
# import os
Copy link
Contributor

Choose a reason for hiding this comment

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

You are going to need to uncomment those.

You also need to modify the path in os.path.abspath so it points to the root of this repo.

You can just do like in conf.py of the CPP_SPM repo

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did change these parts but I suspect I made a mistake and ignore the changes while committing. My bad

@Remi-Gau
Copy link
Contributor

Remi-Gau commented Oct 25, 2020

Looks like you are on good track to me.

You need to make some changes to the conf.py.

Once you are done you can try to build the doc with the following command: to run from within the docs folder.

sphinx-build -b html source build

This creates a set of html file in the build folder you can use to view and browse the doc.

marcobarilari and others added 8 commits October 25, 2020 17:18
Co-authored-by: Remi Gau <remi_gau@hotmail.com>
Co-authored-by: Remi Gau <remi_gau@hotmail.com>
Co-authored-by: Remi Gau <remi_gau@hotmail.com>
Co-authored-by: Remi Gau <remi_gau@hotmail.com>
Co-authored-by: Remi Gau <remi_gau@hotmail.com>
Co-authored-by: Remi Gau <remi_gau@hotmail.com>
Co-authored-by: Remi Gau <remi_gau@hotmail.com>
Co-authored-by: Remi Gau <remi_gau@hotmail.com>
@Remi-Gau
Copy link
Contributor

Also looking around to other repo that use Sphinx it seems that we do not need to commit the content of the build folder as read the docs will take care of building the docs. (I am also trying to figure this out).

So you can remove all that of that from the repo and add the build folder to .gitignore

@Remi-Gau
Copy link
Contributor

Oh by the way you can add several comments at once through the github graphic interface.

I learnt that on the BIDS specs and I still to update their doc there to help others with this.
bids-standard/bids-specification#633 (comment)

@marcobarilari
Copy link
Collaborator Author

ok, I have rebuild sphinx doc with the new config.py edits and add the build folder to .gitignore


.. autofunction:: initPTB()

:func:`initPTB`
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't understand this part, what is it the point to have :func:initPTB`` and .. autofunction:: getExperimentStart after it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
:func:`initPTB`

You are right: this one can be removed and autofunction getExpStart brought up. :-)

@marcobarilari
Copy link
Collaborator Author

I have added all the function in the function_description.rst file LMK if looks ok and what's next (apart from making it nicer :) )

@Remi-Gau
Copy link
Contributor

Remi-Gau commented Oct 31, 2020

I have added all the function in the function_description.rst file LMK if looks ok and what's next (apart from making it nicer :) )

I think you can try using the read the docs website to build the docs so we then have an online version.

If you feel like it.

@marcobarilari
Copy link
Collaborator Author

sure, but somehow the sphinx updates should be in a branch (master) of the repo, shouldn't they? Or there is a way to build the rtd from a PR?

@Remi-Gau
Copy link
Contributor

If it builds locally you can merge this into dev. Then on the read the docs (RTD) in the advanced settings you can set the default branch to build the doc from.

See here: https://readthedocs.org/dashboard/cpp-bids-spm/advanced/

Let me know if you can't access I might need to add you as Maintainers on RTD.

You can also start transferring some content from the old markdown index.md to the rst files if you want. In this PR before merging and trying to build with RTD or in another PR. Up to you. 🚀

@marcobarilari
Copy link
Collaborator Author

See here: https://readthedocs.org/dashboard/cpp-bids-spm/advanced/

I see this (lol)

        \          SORRY            /
         \                         /
          \    This page does     /
           ]   not exist yet.    [    ,'|
           ]                     [   /  |
           ]___               ___[ ,'   |
           ]  ]\             /[  [ |:   |
           ]  ] \           / [  [ |:   |
           ]  ]  ]         [  [  [ |:   |
           ]  ]  ]__     __[  [  [ |:   |
           ]  ]  ] ]\ _ /[ [  [  [ |:   |
           ]  ]  ] ] (#) [ [  [  [ :===='
           ]  ]  ]_].nHn.[_[  [  [
           ]  ]  ]  HHHHH. [  [  [
           ]  ] /   `HH("N  \ [  [
           ]__]/     HHH  "  \[__[
           ]         NNN         [
           ]         N/"         [
           ]         N H         [
          /          N            \
         /           q,            \
        /                           \

@Remi-Gau
Copy link
Contributor

oh dear...

Tried to add you using your github username.

Let me know if this works

@marcobarilari
Copy link
Collaborator Author

it worked! thank you

@marcobarilari
Copy link
Collaborator Author

All right, I added an installation section (also to try sphynx synstax) and going to merge this and publish it to see if it works.

Will add more docs in the future in other PRs, LMK

@marcobarilari marcobarilari marked this pull request as ready for review October 31, 2020 19:25
@marcobarilari marcobarilari merged commit ac0f03b into cpp-lln-lab:dev Oct 31, 2020
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.

3 participants