Skip to content
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

Minor fixes #111

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Minor fixes #111

wants to merge 12 commits into from

Conversation

ocefpaf
Copy link
Contributor

@ocefpaf ocefpaf commented Jul 23, 2024

I was chasing a red-herring due to a bad pinned version in an environment and ended up adding more Pythons to the test matrix here. No need code, just boilerplate updates.

@kthyng
Copy link
Contributor

kthyng commented Jul 26, 2024

@ocefpaf Is some of this the new way to do packaging, in particular removing the setup.* files? Stresses me out. Did you do it correctly?

@ocefpaf
Copy link
Contributor Author

ocefpaf commented Jul 29, 2024

@ocefpaf Is some of this the new way to do packaging, in particular removing the setup.* files?

Yes. See https://peps.python.org/pep-0518/ and https://peps.python.org/pep-0517/ for more info. Python packaging rarely requires a setup.py nowadays.

Stresses me out. Did you do it correctly?

Sorry for the stress. Feel free to close it. Regarding correctness. We can add a packaging tests here, build sdist and wheel for upload, to ensure things work.

@kthyng
Copy link
Contributor

kthyng commented Aug 5, 2024

Sorry for the stress. Feel free to close it. Regarding correctness. We can add a packaging tests here, build sdist and wheel for upload, to ensure things work.

No! That's not what I meant, I meant that I don't feel in a good position to check it and it always feels a bit like magic. You're right, a test would be a good way! And thanks for the resources, I need to keep updating my understanding of how things work.

@ocefpaf
Copy link
Contributor Author

ocefpaf commented Aug 5, 2024

You're right, a test would be a good way!

The release GitHub Actions now:

  • checks the sdist
  • checks wheel
  • checks the manifest
  • installs and run the tests for the wheel

That should help avoid any regressions.

.github/workflows/release.yaml Outdated Show resolved Hide resolved
.github/workflows/test.yaml Outdated Show resolved Hide resolved
.github/workflows/test.yaml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I seem to have not posted this somehow.

.github/workflows/release.yaml Outdated Show resolved Hide resolved
Comment on lines 36 to 41
run: >
cd dist
&& python -m twine check *
&& python -m pip install *.whl
&& cp --recursive ../tests .
&& python -m pytest -rxs tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need the && if you don't fold everything into one line:

Suggested change
run: >
cd dist
&& python -m twine check *
&& python -m pip install *.whl
&& cp --recursive ../tests .
&& python -m pytest -rxs tests
run: |
cd dist
python -m twine check *
python -m pip install *.whl
cp --recursive ../tests .
pytest -rxs tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that, if something errors out, it continues with the next command in your form.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default shell is bash -e, which will fail the whole thing on the first command that fails.

And as a test:

$ bash -e <<EOF
false
echo yes
EOF

prints nothing as the first command failed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants