-
Notifications
You must be signed in to change notification settings - Fork 70
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
Fix CI Pipelines. Drop Python3.6. Bring CI pipelines into harmony with local dev experience. Lint and format entire code base. Accelerate CI pipelines. Update setup.py to correctly define extras packages. #308
Conversation
Codecov Report
Additional details and impacted files |
58675f8
to
eec03c8
Compare
7d36dbc
to
eac05fc
Compare
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.
lgtm, thanks very much for the maintenance! This is a bit more pypi-tools than conda-tools focused, but the repo is pure python, so why not.
I've a couple typos and queries, but merge at will.
@loriab will merge in tonight later when I get home. Thanks for accepting and looking forward to getting the repo a bit leaner on the next rev too :) |
…3.7. Updated CONTRIBUTING.md to contain detailed instructions for developers on how to contribute. Fixed broken code that failed to prepend the "v" to version numbers. Updated CI to run without conda and using only packages defined in setup.py. CI is now much faster and runs the same way for local developers and GitHub Actions. Added test.sh and format.sh to devtools/scripts for easy execution of formatting and testing. Formatted all code with black. Sorted imports with isort. Added pre-commit to repo so code formatting, linting, and testing will all run as part of regular git workflow.
4fddd91
to
35e43d1
Compare
@loriab I still can't merge because of the "required" status that have to pass. I removed theses names from the CI suite but cannot change their "required" status--this is probably an admin-level task. Can you remove these as "required" or update the required tests to the new names I've used? Thanks! |
"required" status has been transferred to new names, so you should be good to merge! |
Description
Fix CI pipelines. Update code to have reliable builds, linting, formatting, etc...
Changelog description
setup.py
. CI is now much faster and runs the same way for local developers and GitHub Actions.test.sh
andformat.sh
todevtools/scripts
for easy execution of formatting and testing.black
. Sorted imports withisort
.git
workflow.More Details
Lint.yaml
github action runs on python3.7. A newer version ofblack
is available on 3.7 that updates some defaults for formatting. This causes CI Lint to automatically fail on the code directly frommaster
that passes lint checks on your local machine. We should drop support for non-supported python versions (python3.6 end of life'd 2021-12-23). Locally you can never install blackv23.1.0
because it doesn't support python3.6. Using supported versions of python avoids these types of tricky version conflicts. We need to move with the whole python community on this--we only shoot ourselves in the foot by supporting dead python versions :) This is why other PRs like this one that should be one-liners come with 25 files changed..github
;P Added detailed instructions for setting up the repo on a local machine for development.test.sh
andformat.sh
scripts todevtools/scripts
This makes it easy for new devs to quickly and idiomatically run tests and format/lint their code. Also quicker for experienced devs too :)v23.10++
) to avoid these discrepancies between local lint passing and CI failing. I have updated this dependency insetup.py
.viz
extras/tests/test_molecule.test_show()
did not work becauseipykernel
is not installed and needs to be to work withnglview
test_pubchem_multiout_g()
so it passes PubChem was no longer returning the193687
molecule due to changing its name. Added a new molecule instead.codecov.yaml
to be valid yaml and renamed (removed.
) so it works with actions.Did not do
flake8
for much stricter code quality. Repo has too many violations for now. That should be a separate PR.Future Work
setup.py
,setup.cfg
, and move to fully modernpyproject.yaml
. Also encapsulates venv management--easier for new devs, I think. This will also avoid issues like different linter versions causing linting to pass on some machines, fail on others, and conflict with CI.Status