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] Enable table text wrap and link docstrings to code on GitHub #1258

Merged
merged 5 commits into from
Aug 27, 2018

Conversation

tsalo
Copy link
Collaborator

@tsalo tsalo commented Aug 23, 2018

Changes proposed in this pull request

  • Closes [DOC] Enable text wrap in documentation tables? #1257.
  • Adds theme_overrides.css file recommended here to enable text wrap in tables on RTD site. Currently, tables with a lot of text are just very wide and require horizontal scrolling.
  • Adds github_link function which, when combined with sphinx.ext.linkcode, links function and class docstring documentation to the actual code on GitHub. Currently, sphinx.ext.viewcode is used to show a local snapshot of the function or class.
  • Changes CircleCI pytest configuration to ignore the docs folder using the full path rather than the relative path.

Documentation that should be reviewed

Changes to the docs configuration file (conf.py) to use the theme override (for text wrapping) and the link code resolver (to link docstrings to GitHub) should be reviewed.

@oesteban
Copy link
Member

This https://4031-53175327-gh.circle-artifacts.com/0/home/circleci/out/docs/usage.html is indeed much better 👍

There is a test failing because an import in the conf.py (which is surprising, because --ignore=docs should dismiss that file).

==================================== ERRORS ====================================
________________________ ERROR collecting docs/conf.py _________________________
/root/src/fmriprep/docs/conf.py:27: in <module>
    from github_link import make_linkcode_resolve
E   ModuleNotFoundError: No module named 'github_link'
!!!!!!!!!!!!!!!!!!! Interrupted: 1 errors during collection !!!!!!!!!!!!!!!!!!!!
=========================== 1 error in 4.37 seconds ============================
Exited with code 2

So 1) we should not import unavailable modules, and 2) why the conf.py is not ignored?

@tsalo
Copy link
Collaborator Author

tsalo commented Aug 23, 2018

github_link comes from docs/sphinxext/github_link.py, which is added to the path here:

https://github.com/poldracklab/fmriprep/blob/65ec2adca851305ee0020f7ff79cb0956d569534/docs/conf.py#L24-L27

The same approach has worked well in other places, like tedana, but I think CI is raising an error because the tests are being run from a different folder. I can replace sys.path.append(os.path.abspath('sphinxext')) with sys.path.append(os.path.join(os.path.dirname(__file__), 'sphinxext')) so that it adds the folder docs/sphinxext regardless of where conf.py is called from. When I do that and run pytest docs/conf.py from the fmriprep directory, that error disappears. Still, though, I don't think it actually solves anything, because docs should be ignored by the tests and the doc pages should be built from within the docs folder...

@tsalo
Copy link
Collaborator Author

tsalo commented Aug 24, 2018

Good news. It looks like the problem was just that docs wasn't being properly ignored, because relative paths (e.g., docs) don't work when you call pytest on a full path (e.g., /root/src/fmriprep/). After ignoring the full /root/src/fmriprep/docs rather than just docs, it seems to work fine. On the bright side, the same issue doesn't seem to extend to setup.py, or pytest ignores setup.py by default (not sure which).

@effigies
Copy link
Member

It doesn't look like the docs are linking back to GitHub. Is this expected until it's merged?

@tsalo
Copy link
Collaborator Author

tsalo commented Aug 27, 2018

Merging should make that happen. Until then it should just be using sphinx.ext.viewcode to view the snippers on the RTD site.

@effigies
Copy link
Member

Let's give it a shot. Thanks.

@effigies effigies merged commit f5b502c into nipreps:master Aug 27, 2018
DimitriPapadopoulos added a commit to DimitriPapadopoulos/fmriprep that referenced this pull request Nov 21, 2023
Copied from scikit-learn 1.3.2.

File initially copied in nipreps#1258.
DimitriPapadopoulos added a commit to DimitriPapadopoulos/fmriprep that referenced this pull request Nov 21, 2023
As far as I can see, we had just added a few lines to tell this vendored
script originates in scikit-learn.

File initially copied in nipreps#1258, and not changed since.
effigies added a commit that referenced this pull request Nov 21, 2023
<!--
Text in these brackets are comments, and won't be visible when you
submit your pull request.
If this is your first contribution, please take the time to read these,
in particular the comment
beginning "Welcome, new contributors!".
-->

## Changes proposed in this pull request

As far as I can see, we had just added a few lines to tell this vendored
script originates in scikit-learn.
    
File initially copied in #1258, and [not changed
since](https://github.com/nipreps/fmriprep/commits/master/docs/sphinxext/github_link.py),
except for #3097 which I shouldn't have applied to a vendored file.
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.

[DOC] Enable text wrap in documentation tables?
3 participants