Skip to content

DOCS: Self-host docs using GitHub pages#266

Merged
josephmje merged 34 commits intoTIGRLab:masterfrom
josephmje:docs/sphinx_init
May 5, 2020
Merged

DOCS: Self-host docs using GitHub pages#266
josephmje merged 34 commits intoTIGRLab:masterfrom
josephmje:docs/sphinx_init

Conversation

@josephmje
Copy link
Copy Markdown
Contributor

@josephmje josephmje commented Mar 10, 2020

Changes proposed in this PR

  • add Sphinx configuration
  • update CircleCI config to automatically update docs
  • string formatting to Python 3 f-strings
  • automatic code formatting as a results of the following VSCode Settings
{
  "python.pythonPath": "/Users/michaeljoseph/.pyenv/versions/datman_venv/bin/python",
  "python.linting.flake8Enabled": true,
  "python.linting.flake8Args": [
    "--config=setup.cfg"
  ],
  "python.formatting.provider": "black",
  "python.formatting.blackArgs": [
    "--line-length=80"
  ],
  "python.sortImports.args": [
    "-sp setup.cfg"
  ],
  "[python]": {
    "editor.formatOnSave": true,
    "editor.codeActionsOnSave": {
      "source.organizeImports": true
    }
  },
  // Configure editor settings to be overridden for [yaml] language
  "[yaml]": {
    "editor.insertSpaces": true,
    "editor.tabSize": 2,
  }
}

To Do

  • check that fingerprints are correct in CircleCI config

@josephmje josephmje marked this pull request as ready for review March 11, 2020 16:28
@auto-assign auto-assign bot requested review from DESm1th, edickie and gabiherman March 11, 2020 16:28
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 11, 2020

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #266   +/-   ##
=======================================
  Coverage   30.66%   30.66%           
=======================================
  Files          56       56           
  Lines        8639     8639           
=======================================
  Hits         2649     2649           
  Misses       5990     5990           
Flag Coverage Δ
#unittests 30.66% <0.00%> (ø)

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 8f3e344...8f3e344. Read the comment docs.

@pep8speaks
Copy link
Copy Markdown

pep8speaks commented Apr 3, 2020

Hello @josephmje, Thank you for updating!

Line 111:13: W503 line break before binary operator
Line 112:13: W503 line break before binary operator

Line 280:5: W503 line break before binary operator
Line 281:5: W503 line break before binary operator
Line 282:5: W503 line break before binary operator
Line 283:5: W503 line break before binary operator
Line 284:5: W503 line break before binary operator
Line 285:5: W503 line break before binary operator
Line 290:5: W503 line break before binary operator
Line 291:5: W503 line break before binary operator
Line 292:5: W503 line break before binary operator
Line 293:5: W503 line break before binary operator
Line 294:5: W503 line break before binary operator
Line 295:5: W503 line break before binary operator
Line 300:5: W503 line break before binary operator
Line 301:5: W503 line break before binary operator
Line 302:5: W503 line break before binary operator
Line 303:5: W503 line break before binary operator
Line 304:5: W503 line break before binary operator
Line 305:5: W503 line break before binary operator
Line 306:5: W503 line break before binary operator
Line 307:5: W503 line break before binary operator
Line 308:5: W503 line break before binary operator
Line 309:5: W503 line break before binary operator
Line 310:5: W503 line break before binary operator

Line 1008:13: W503 line break before binary operator

To test for issues locally, pip install flake8 and then run flake8 datman.

Comment last updated at 2020-04-21 13:16:08 UTC

@josephmje
Copy link
Copy Markdown
Contributor Author

josephmje commented Apr 3, 2020

docs are being built here!

Comment thread datman/utils.py Outdated
raise MetadataException("Can't return BIDs blacklist info without "
"dashboard integration")
raise MetadataException(
"Can't return BIDs blacklist info without " "dashboard integration"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not a big deal, but this should probably be one string if on one line now

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

thanks for finding this. will fix!

Comment thread docs/requirements.txt Outdated
Comment on lines +1 to +2
git+https://github.com/AleksandarPetrov/napoleon.git@0dc3f28a309ad602be5f44a9049785a1026451b3#egg=sphinxcontrib-napoleon
git+https://github.com/rwblair/sphinxcontrib-versioning.git@39b40b0b84bf872fc398feff05344051bbce0f63#egg=sphinxcontrib-versioning
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a reason to use these specific commits on these repos instead of just the plain old sphinxcontrib-napoleon and sphinxcontrib-versioning?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the napoleon one has a feature for rending the Parameters header in the docstring that hasn't been merged yet. but I'll remove the versioning one.

DESm1th
DESm1th previously approved these changes Apr 21, 2020
Copy link
Copy Markdown
Contributor

@DESm1th DESm1th left a comment

Choose a reason for hiding this comment

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

Nice work! Thanks for putting this together and pushing us toward using the newer style f strings. I think moving forward we should definitely use them, but maybe make an exception and use .format for really large strings though. For me at least it's easy to lose track of variables needed to fill out a large string and accidentally delete or rename something, but format helps a bit in these cases by keeping them grouped at the end. What do you think? Maybe it doesn't make much a difference (it shouldn't if we have tests over code in these instances, haha).

@josephmje
Copy link
Copy Markdown
Contributor Author

Nice work! Thanks for putting this together and pushing us toward using the newer style f strings. I think moving forward we should definitely use them, but maybe make an exception and use .format for really large strings though. For me at least it's easy to lose track of variables needed to fill out a large string and accidentally delete or rename something, but format helps a bit in these cases by keeping them grouped at the end. What do you think? Maybe it doesn't make much a difference (it shouldn't if we have tests over code in these instances, haha).

Thanks Dawn. I made the switch the improve readability and cut down on some of the line lengths. But I used an automatic converter to do it and wasn't discriminating between really large strings. That's a good point. I'm in favour of any option that would help with understanding the code.

@DESm1th
Copy link
Copy Markdown
Contributor

DESm1th commented Apr 21, 2020

Yeah! I don't think we actually have very many strings large enough for it to matter and none of the strings you updated are all that long. More tests is definitely a better solution than deciding on a case by case basis whether to switch to the better f-string format haha.

@josephmje josephmje merged commit 66519f4 into TIGRLab:master May 5, 2020
@josephmje josephmje deleted the docs/sphinx_init branch August 7, 2020 18:31
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.

4 participants