-
Notifications
You must be signed in to change notification settings - Fork 132
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
Flake & Make #273
Flake & Make #273
Changes from 11 commits
572c6a8
b5301b8
a4d5314
f4c45d0
90b70c8
1d9903e
ce41ea6
a892766
6e4df41
73d1d0c
f0e9b76
00852fc
405a6fe
c709869
06ac7f5
c4ea3fa
30a62bc
74ca33e
69dcbea
34a1464
084e2ce
ebb13f1
bbb6b8a
72a78a0
008fb5a
31a4612
cd65173
1f1e246
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
[flake8] | ||
tzaffi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
ignore = | ||
E203, | ||
E241, | ||
W291, | ||
E302, | ||
F403, | ||
F405, | ||
E501, | ||
W503, | ||
E741, | ||
|
||
per-file-ignores = | ||
pyteal/ir/ops.py:E221 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
pip: | ||
pip install -e . | ||
tzaffi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
pip-development: pip | ||
tzaffi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
pip install -e.[development] | ||
|
||
pip-docs: pip-development | ||
pip install -r docs/requirements.txt | ||
pip install doc2dash | ||
|
||
pip-wheel: | ||
pip install wheel | ||
|
||
bdist-wheel: | ||
python setup.py sdist bdist_wheel | ||
|
||
docs: | ||
tzaffi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
rm -rf docs/pyteal.docset | ||
cd docs && \ | ||
make html && \ | ||
doc2dash --name pyteal --index-page index.html --online-redirect-url https://pyteal.readthedocs.io/en/ _build/html && \ | ||
tar -czvf pyteal.docset.tar.gz pyteal.docset | ||
|
||
build: | ||
python -m scripts.generate_init --check | ||
tzaffi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
ALLPY = docs examples pyteal scripts tests *.py | ||
black: | ||
black --check $(ALLPY) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was going into my virutal env directory and failing because of that, so I wanted to explicitly define what is being checked. It's also handy to use this for the other linters ( Another approach is to specifically exclude certain folders with But admittedly, the downside is that if you add another top-level directory, we wouldn't be formatting it by default. I don't feel strongly about this. LMK |
||
|
||
flake8: | ||
flake8 $(ALLPY) | ||
|
||
MYPY = pyteal scripts | ||
mypy: | ||
mypy $(MYPY) | ||
|
||
lint: black flake8 mypy | ||
tzaffi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
test-unit: | ||
tzaffi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
pytest | ||
|
||
build-and-test: build lint test-unit | ||
|
||
# Extras: | ||
coverage: | ||
pytest --cov-report html --cov=pyteal | ||
|
||
.PHONY: build docs |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,3 @@ | ||
import pytest | ||
|
||
from .. import * | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,3 @@ | ||
import pytest | ||
|
||
from .. import * | ||
|
||
# this is not necessary but mypy complains if it's not included | ||
|
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 For https://www.flake8rules.com/rules/F405.html and https://www.flake8rules.com/rules/F403.html, I'm imagining we'd prefer to avoid the violation in source code. Does it work for you to limit ignoring F403 + F405 to
*_test.py
andexamples/
?From the previous attempt, there's an example ignoring
*_test.py
: https://github.com/algorand/pyteal/pull/213/files#diff-6951dbb399883798a226c1fb496fdb4183b1ab48865e75eddecf6ceb6cf46442R11.Running locally, I see there's a F403 violation in source code. I didn't look closely enough to determine how easily it can be avoided vs adding inline ignore. I can take a deeper look if you'd like - let me know.
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'll research this some more and update here.
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.
When I first joined I didn't like our profligate use of * imports in pyteal. But the more I use pyteal, the more this convention makes sense to me. We have so many constructs that are typically used in pyteal, that a standard pyteal program would have 10 or more objects being imported.
An alternate approach in such situations (where a library with many constructs is used) is to abbreviate the imported module and then always refer to it. For example:
we could follow a similar convention and start enforcing it here with usages such as below (notice, we are already implicitly recommending this approach with
abi
):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.
A few thoughts from my side:
pandas as pd
approach. I think it'd help disambiguate PyTeal constructs that match Python standard library (e.g.Array
).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.
There's 3 separate issues:
import pyteal as pt
or similar, I wouldn't mind.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'll go with the modifying the tests files only and adding flake8 exceptions for other files.
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.
hehe... that resulted in 6000+ changes
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 the changes here - I think it's reasonable starting point and we can iterate over time. Resolving.