-
Notifications
You must be signed in to change notification settings - Fork 7
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
Update packaging & switch to versioningit #74
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #74 +/- ##
==========================================
- Coverage 99.74% 99.73% -0.01%
==========================================
Files 5 5
Lines 385 383 -2
==========================================
- Hits 384 382 -2
Misses 1 1 ☔ View full report in Codecov by Sentry. |
Hello @IAlibay! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2023-12-23 14:35:15 UTC |
@fiona-naughton this would be a good one for pair review - it's a rather odd packaging setup so it might end up being quite informative. |
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.
If feedback from a passerby is welcome, seems good to me. I have a couple selfish curiosities, no criticisms or suggested improvements though
@@ -16,7 +16,7 @@ concurrency: | |||
|
|||
defaults: | |||
run: | |||
shell: bash -l {0} |
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.
Just curious why this is no longer necessary? It's bit magical to me but I often ran into problems without it
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 thought I had written some answer here - sorry about the delay. The answer is that we're using setup-python
that happens to be quite sensitive to login shells on macOS (compared to mamba which in my experience is sensitive to non-login shells 😓 ). So you end up needing a pure bash
shell, otherwise it'll pick up system python in subsequent parts of the workflow.
.github/workflows/gh-ci.yaml
Outdated
which pip | ||
conda info | ||
conda list | ||
pip install pytest pytest-xdist pytest-cov codecov |
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.
Very minor - if you using CodeCov's official-ish action, I don't think you need to install their Python package
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.
thanks!
EDR_IRREG = resource_filename(__name__, 'data/irregular.edr') | ||
EDR_IRREG_XVG = resource_filename(__name__, 'data/irregular.xvg') | ||
EDR_IRREG_UNITS = resource_filename(__name__, 'data/irregular_units.p') | ||
EDR = (_data_ref / 'cat_small.edr').as_posix() |
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.
Purely out of curiousity, would
EDR = (_data_ref / 'cat_small.edr').as_posix() | |
EDR = str(_data_ref / 'cat_small.edr') |
not wort for Windows-y reasons?
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.
If I remember correctly as_posix just returns str + a slahes replacement. I think I'm mostly being overly cautious - I suspect it would work fine either way.
Thanks for the review @mattwthompson ! |
Fixes #73