-
-
Notifications
You must be signed in to change notification settings - Fork 670
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
Restructure repository #1669
Restructure repository #1669
Conversation
src/*/.tox | ||
src/*/.coverage | ||
src/*/.coverage.* |
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 3 patterns are already listed above without any directory, so they don't need to be listed per-directory as well.
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.
Looks good - high churn, but mostly uncontroversial stuff. I saw a couple of minor docs issues and a missing file on the meta package which I've corrected.
I've also flagged two issues inline. One is "strong opinion, weakly held" (passing in --output
building into a single /dist
folder); the other is a "weak opinion, weakly held" (making tox -e package
a package-level tox definition). I'm marking the PR as approved; I'm totally OK with this being merged without further changes, but if either of those points seem reasonable to you, I'd be OK with the change - and as long as CI still passes, a merge.
e5fa757
to
594d2d4
Compare
Codecov Report
|
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.
Looks good, and the 0.0.1rc10 release appears to work in my testing.
The only problem I've spotted is tied to the one piece of CI that we can't exercise - the release script.
- name: Extract ${{ matrix.package }} | ||
run: | | ||
mkdir dist | ||
mv packages/$(echo ${{ matrix.package }} | sed 's/_/?/')-[0-9]* dist | ||
mv */dist/$(echo ${{ matrix.package }} | sed 's/_/?/')-[0-9]* dist |
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 think there might be an analogous change missing from publish.rst?
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.
After discussion in person, turns out this isn't an issue on publish, because in the publish action, the artefact is coming from the GitHub Release, rather than the CI artefact,
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.
After discussion in person, looks like the problem I flagged isn't actually a problem; so I think we're good to go!
- name: Extract ${{ matrix.package }} | ||
run: | | ||
mkdir dist | ||
mv packages/$(echo ${{ matrix.package }} | sed 's/_/?/')-[0-9]* dist | ||
mv */dist/$(echo ${{ matrix.package }} | sed 's/_/?/')-[0-9]* dist |
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.
After discussion in person, turns out this isn't an issue on publish, because in the publish action, the artefact is coming from the GitHub Release, rather than the CI artefact,
Fixes #1627.
I've kept most of the mechanical changes in the first two commits, so you can exclude those to see the interesting stuff.
I've reviewed all appearances of "src" and "../" in the repository, so hopefully I haven't missed anything. But I haven't touched release.sh, because it's no longer being used by CI, and if we can move to setuptools-scm then it'll no longer have any use at all and can be removed.
PR Checklist: