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

Check todo in docs #71

Merged
merged 25 commits into from Nov 5, 2021
Merged

Conversation

FernandoDavis
Copy link
Contributor

This PR addresses the todos in the contributing, style, and operators sections of the documentation. Makes the install a fork tutorial instead, adds references to style guides used, and creates docstrings for marked wrapper methods.

It is still a draft waiting for some edits in the installation section.

@FernandoDavis
Copy link
Contributor Author

The conflict is most likely because both Brendt and I removed the todo snippet. Should be an easy fix.

@bwohlberg
Copy link
Collaborator

This addresses #15, right? Any other issues as well?

@FernandoDavis
Copy link
Contributor Author

This PR addresses #15, #60, #61, and #63.

@bwohlberg
Copy link
Collaborator

It completely resolves all of them so that they can all be closed when this PR is merged?

- ``install_conda.sh``: install ``miniconda``
(needed if conda is not already installed on your system).
- ``conda_env.sh``: install a ``conda`` environment
with all SCICO dependencies. For GPU installation, see ``conda_env.sh -h``.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The instructions may change per discussion with @Michael-T-McCann

Copy link
Collaborator

Choose a reason for hiding this comment

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

These should be replace with the pip instructions.

@Michael-T-McCann
Copy link
Contributor

"For Developers" and "Installing Dependencies" are currently showing up as top level headings (in the left side navigation bar). I don't think they warrant that and suggest removing them.

@Michael-T-McCann
Copy link
Contributor

Clicking on "important classes" in the left side navigation bar takes you to a slightly odd/useless page, unlike any of the other links there.

::
::

git checkout -b name-of-change
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we have further guidelines on branch names somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! Thank you for spotting that, I will update it with the guideline.


Unicode variable names are allowed for internal usage (e.g. for Greek characters for mathematical symbols), but not as part of the public interface for functions or methods.


Naming
------

Naming conventions for the different programming components are as follows:
Naming conventions adhering to the `Google naming conventions <https://google.github.io/styleguide/pyguide.html#3164-guidelines-derived-from-guidos-recommendations>`_ for the different programming components are as follows:
Copy link
Contributor

Choose a reason for hiding this comment

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

While I appreciate the effort it took to work these docs links into the text, I found them a bit cumbersome to read. It also raises the question: "Do we follow all of the, .e.g., google string conventions, or only the ones we mention specifically?".

I propose a more formulaic approach to these sections:
"Strings
We follow the Google string conventions. Notably, prefer to use Python f-strings, rather than .format or % syntax. For example, ..."

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this suggestion, but it would be good to keep the links.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, that was my intention but it wasn't clear from my example.

@codecov
Copy link

codecov bot commented Nov 3, 2021

Codecov Report

Merging #71 (79f8372) into main (384a2bf) will not change coverage.
The diff coverage is n/a.

❗ Current head 79f8372 differs from pull request most recent head ab45b70. Consider uploading reports for the commit ab45b70 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main      #71   +/-   ##
=======================================
  Coverage   89.12%   89.12%           
=======================================
  Files          43       43           
  Lines        2978     2978           
=======================================
  Hits         2654     2654           
  Misses        324      324           
Flag Coverage Δ
unittests 89.12% <ø> (ø)

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

Impacted Files Coverage Δ
scico/_generic_operators.py 91.89% <ø> (ø)

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 384a2bf...ab45b70. Read the comment docs.

@bwohlberg bwohlberg added the documentation Improvements or additions to documentation label Nov 3, 2021
@FernandoDavis
Copy link
Contributor Author

There are two things that need fixing:

  • for some reason it makes a tab called Installation?
  • I cant hyperlink to the installing a development version in contributing from install.rst

If someone can help with those two things I would appreciate it cause I am not sure what's the issue.

@bwohlberg
Copy link
Collaborator

@Michael-T-McCann : Looks good to me now, but it wouldn't hurt for you to take another look over it to confirm that your suggested changes as as intended.

@Michael-T-McCann
Copy link
Contributor

There are two things that need fixing:

* for some reason it makes a tab called Installation?

* I cant hyperlink to the installing a development version in contributing from install.rst

If someone can help with those two things I would appreciate it cause I am not sure what's the issue.

Working on these now.

@Michael-T-McCann Michael-T-McCann enabled auto-merge (rebase) November 5, 2021 15:37
@Michael-T-McCann Michael-T-McCann merged commit 3852542 into main Nov 5, 2021
@bwohlberg bwohlberg deleted the FernandoDavis/check_TODO_in_docs branch November 5, 2021 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
3 participants