Ape17 #290
Ape17 #290
Conversation
Codecov Report
@@ Coverage Diff @@
## main #290 +/- ##
==========================================
- Coverage 75.50% 75.46% -0.05%
==========================================
Files 44 44
Lines 5128 5119 -9
==========================================
- Hits 3872 3863 -9
Misses 1256 1256
Continue to review full report at Codecov.
|
|
As per the instructions, @astrofrog please have a look. |
There was a problem hiding this comment.
Many minor comments, some nitpicks, some are real issues. When these are addressed I would still suggest to wait for another review from the others as I'm not sure I picked up all the issues.
For my CI related comments, I would suggest that it's fine to decouple those from this refactor PR and do separately.
|
Nice! I will review once Brigitta's comments are addressed. Thanks! |
@pllim - I've finally found the time to address Brigitta's comments. Thanks |
There was a problem hiding this comment.
You might want to audit your MANIFEST.in. You don't have directories like cextern nor scripts, among other things, so there is no need to declare them in your packaging manifest. But that is out of scope here (#302).
Comments below. Thanks!
While you are at this, might as well replace distutils? I see this in your test log: DeprecationWarning: distutils Version classes are deprecated
|
@pllim - thanks for the prompt code review. Re-worked done including the |
|
Almost there, just two more questions above. Thanks! |
Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
|
Nice work, nothing feels better than a -1K+ diff for a PR :) |
This PR addresses the following:
astropylimit in install dependencies #254