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

Add GitHub Actions and tox #66

Merged
merged 5 commits into from
Apr 2, 2021
Merged

Add GitHub Actions and tox #66

merged 5 commits into from
Apr 2, 2021

Conversation

altendky
Copy link
Collaborator

@altendky altendky commented Apr 1, 2021

No description provided.

@altendky
Copy link
Collaborator Author

altendky commented Apr 1, 2021

This is pretty minimal, but does run the tests. Maybe double check it runs all of the ones you want... The commented out stuff should come back with #61 so I'm being a bit dirty and leaving it there. We can add various checks separately.

@altendky
Copy link
Collaborator Author

altendky commented Apr 1, 2021

I may as well link Parquery/pylddwrap#16 (comment) where there is a little commentary about the structure. Or we can just chat about it as it interests you. This is pretty much copied from pylddwrap since that is my most recent effort and also a Linux-only CI setup like I think plotman can start with. Correct me if you expect it to work on other platforms and we can add those in pretty easily.

@altendky altendky marked this pull request as ready for review April 1, 2021 04:02
@jkbecker
Copy link
Contributor

jkbecker commented Apr 1, 2021

Looks nice! I feel like this should be merged before #61, then I can pull it and make sure it also works with the directory structure I built there.

Re: tox: The official blockchain requires Python 3.7, is there a reason to bother with versions below that for plotman?

@altendky
Copy link
Collaborator Author

altendky commented Apr 1, 2021

I tend towards supporting all supported Pythons (and in legacy cases also Python 2.7 when it isn't too onerous). That's certainly a reasonable argument about 3.7 though. On the flip side, didn't you just run plotman on a system without chia-blockchain? :] I'm trying to think what the big reasons are for requiring 3.7 in general. I think asyncio grew some pleasant stuff. from __future__ import annotations would be relevant for type hinting (you don't have to quote forward references). Probably something I'm forgetting... Anyways, it's probably not a big deal either way. @ericaltendorf can flip the coin. (or pretend they did and just tell us the answer)

@altendky altendky merged commit 0e4076c into development Apr 2, 2021
@altendky altendky deleted the gha branch April 2, 2021 21:52
@altendky
Copy link
Collaborator Author

altendky commented Apr 3, 2021

Oh yeah, subprocess.run() got some nice tweaks too.

Changed in version 3.7: Added the text parameter, as a more understandable alias of universal_newlines. Added the capture_output parameter.

You may yet get me on the 3.7+ bandwagon. :]

@jkbecker
Copy link
Contributor

jkbecker commented Apr 3, 2021 via email

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