-
Notifications
You must be signed in to change notification settings - Fork 1
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
Initialize analysis incubator repository #1
Conversation
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 took a first pass review. Tested pre-commit, sphinx, and test analysis and it all works. Left you some minor comments.
Two more things:
- Do we want to replace the logo to use the new design?
- Could we add some text instructing user to set up pre-commit?
Here is what we wrote in clowder doc
We use pre-commit to run linting and formatting checks before committing code. To install pre-commit, run pip install pre-commit
and then pre-commit install
in the root directory of the repository. This will install the pre-commit hooks and run them on every commit. If any of the hooks fail, the commit will be aborted.
Once ran, the hooks will be in place for all future commits and branches since they are stored in the .git/hooks. To run the hooks manually, run pre-commit run --all-files
. To skip the hooks, run git commit --no-verify
(or click on cog in PyCharm and uncheck Run Git hooks).
See .pre-commit-config.yaml for the list of hooks that are run. The hooks are run in the order they are listed in the file. The hooks are run on all files that are staged for commit. To run the hooks on all files, run pre-commit run --all-files
.
Hooks include:
- Python style checks and formatting using black and flake8.
Yes, we should probably use the new logo. For your other point, it's not clear where that should go. The README.rst is more about installing the package for running, not for development. The precommit hook is more about development and that should probably go wherever we put the developer documentation. Maybe a wiki page in IN-CORE docs? |
I will update the logo since we are changing it for the project, but it's not actually used anywhere - FYI |
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 did a project wide search of "pyincore" and find a few more missing spots. Besides that all seems to look good.
docs/README.md
Outdated
|
||
### Installation | ||
|
||
Clone the code from pyincore repository [git](https://github.com/IN-CORE/pyincore-incubator.git) |
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.
Clone the code from pyincore-incubator
?
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.
fixed
recipes/meta.yaml
Outdated
license: MPL-2.0 | ||
summary: 'Python library for IN-CORE (Interdependent Networked Community Resilience Modeling Environment)' | ||
description: 'pyIncore is a component of IN-CORE. It is a python package consisting of two primary components: | ||
1) a set of service classes to interact with the IN-CORE web services, and 2) IN-CORE analyses. The pyIncore |
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.
Do we need to adjust the description here, or is it intended to describe pyincore?
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.
Fixed this too
requirements.sh
Outdated
EOF | ||
|
||
# create the environment.yml file for conda. This is intended to setup a conda environment for | ||
# development on pyincore_viz. |
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.
pyincore_viz?
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.
another good find.
setup.py
Outdated
}, | ||
project_urls={ | ||
"Bug Reports": "https://github.com/IN-CORE/pyincore/issues", | ||
"Source": "https://github.com/IN-CORE/pyincore", |
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.
project_urls points to pyincore still
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.
dang, good find. I'll fix that
Tested docker container and local env for sphinx, also tested example analysis. All work. |
@@ -0,0 +1,83 @@ | |||
pyincore-incubator | |||
======== | |||
|
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.
This PR creates the basic incubator repository for new analyses not ready to be included in the core set of analyses. Included are: