-
Notifications
You must be signed in to change notification settings - Fork 17
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
Conversation
The conflict is most likely because both Brendt and I removed the todo snippet. Should be an easy fix. |
This addresses #15, right? Any other issues as well? |
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``. | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
"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. |
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. |
docs/source/contributing.rst
Outdated
:: | ||
:: | ||
|
||
git checkout -b name-of-change |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
docs/source/style.rst
Outdated
|
||
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: |
There was a problem hiding this comment.
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, ..."
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 Report
@@ Coverage Diff @@
## main #71 +/- ##
=======================================
Coverage 89.12% 89.12%
=======================================
Files 43 43
Lines 2978 2978
=======================================
Hits 2654 2654
Misses 324 324
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There are two things that need fixing:
If someone can help with those two things I would appreciate it cause I am not sure what's the issue. |
@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. |
Working on these now. |
…tall guide, and other minor inconsistencies.
79f8372
to
ab45b70
Compare
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.