-
Notifications
You must be signed in to change notification settings - Fork 8
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
Basic Project Setup + Migrating Code from PySDK PR #298 #1
Conversation
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.
@tzaffi Thanks for tying together a number of threads to bring graviton to life. ☕
For review completeness: I'm on-board to try the proposed versioning scheme though I have lingering concerns. Since we can more freely make changes here, let's try it out and assess over time.
Rationale:
- It's possible unicode characters interact poorly with other systems (e.g. build systems) and/or are difficult to parse when not presented as emojis (e.g.
git tag --list
vsgit --no-pager tag --list
). - The convention does not support signaling when a breaking change is made. The lack of signaling makes it harder to know when it's safe to upgrade.
We should keep the new versioning scheme at least through April 1, 2022 |
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.
Comments that didn't make sense on a particular line:
- I think the animal emoji versioning scheme only make sense if it's mapped directly to a semver version (e.g. 🐈 = 0.1.0). I think it's good marketing, but it should not replace semver and traditional versioning.
- I'm in favor of having
*_test.py
unit tests that live in the same directory as implementation code. They are very convenient and act as a constant reminder to write unit tests for your code.
I only shared the above two comments because the context of this repo seems to be to establish best practices for future repos. If that were not the case, my opinions on these issue would not be very strong.
Makefile
Outdated
pip: | ||
pip install -e . | ||
|
||
pip-test: pip |
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 can accept this, though I'd suggestion development
or similar instead of just test
. The dev process involves more than just tests, there are also linters, formatters, etc. that help us write code.
TEAL Blackbox in Graviton
This PR takes over from the now defunct PySDK PR #298
TLDR; Dry Run a Sequence of Inputs on Apps/LogicSigs, View Stats, and make Assertions on Behavior
Please refer to the TEAL Blackbox HowTo / README for full details
A Place to Test Our Python Repo Best Practices
Since this is a brand new repo this is a good place to set some new conventions that our other Python repos should eventually follow also. This PR does not claim to have achieved such a state (especially in light of issues #4 and #7).
Some conventions are implicitly advanced in this PR:
requirements.txt
:extras_require
(e.g.extras_require={"test": "pytest==7.1.1"},
) insetup.py
pip install -e.[test]
act
to simulate github actions locallyMakefile
to summarize basic setup/testing commands, and incorporate these commands in CI configsNo longer considered a "best practice"tests
should contain all unit and integration tests (as opposed to pairing*_test.py
files next to the file they are testing)setup.cfg
whenever possibleblack --check
andflake8
Testing
Simulating Github Actions Locally (on a Mac)
TODO
CI / Best Practices
act
Only Dependencies #2mypy
andblack
andflake8
#4Features / Improvements
Migrate Sandbox Code from PySDK and Run All Tests
Testing is cribbing off of PyTeal's "Run blackbox tests in CI" PR
First successful docker build
First cached docker build