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

Flake & Make #273

Merged
merged 28 commits into from
Apr 19, 2022
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
572c6a8
initial stab - without applying flake
Apr 12, 2022
b5301b8
bring in examples
Apr 12, 2022
a4d5314
pull optimizer.* up a level
Apr 12, 2022
f4c45d0
more flakes
Apr 12, 2022
90b70c8
remove future changes
Apr 12, 2022
1d9903e
add make instructions
Apr 12, 2022
ce41ea6
Merge remote-tracking branch 'origin/master' into make-and-flake
Apr 12, 2022
a892766
apply lint to tests/ too
Apr 12, 2022
6e4df41
EOF
Apr 12, 2022
73d1d0c
narrow the ignores per CR suggestion
Apr 13, 2022
f0e9b76
`make docs` idempotent per CR suggestion
Apr 13, 2022
00852fc
Remove redundant test imports
michaeldiamant Apr 14, 2022
405a6fe
per CR suggestions
Apr 14, 2022
c709869
per CR suggestions
Apr 14, 2022
06ac7f5
Merge branch 'make-and-flake_remove_redundant_imports' of https://git…
Apr 14, 2022
c4ea3fa
Remove redundant test imports (#277)
michaeldiamant Apr 14, 2022
30a62bc
no more blanket import * ignores - ignore in library code, but use pt…
Apr 14, 2022
74ca33e
exclude module_test.py from import * disallowance
Apr 14, 2022
69dcbea
mardkownlint for the README.md
Apr 14, 2022
34a1464
fix the flake8 guidance
Apr 14, 2022
084e2ce
mypy pyteal only in readme
Apr 14, 2022
ebb13f1
per CR catch
Apr 14, 2022
bbb6b8a
better formatting
Apr 14, 2022
72a78a0
Merge branch 'master', remote-tracking branch 'origin' into make-and-…
Apr 14, 2022
008fb5a
revert per CR comments
Apr 19, 2022
31a4612
revert per CR comments
Apr 19, 2022
cd65173
move ignore to .flake8
Apr 19, 2022
1f1e246
Merge remote-tracking branch 'origin/master' into make-and-flake
Apr 19, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .flake8
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[flake8]
Copy link
Contributor

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 and examples/?

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.

~/dev/pyteal make-and-flake *1 !1 ?6 ────────────────────────────────────────────────────────────────────────── ✘ 2|1 5s  pyteal 09:53:33 AM
❯ make flake8 | grep -v -E 'test|examples'
pyteal/__init__.py:1:1: F403 'from .ast import *' used; unable to detect undefined names
pyteal/__init__.py:3:1: F403 'from .ir import *' used; unable to detect undefined names
make: *** [flake8] Error 1

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'll research this some more and update here.

Copy link
Contributor Author

@tzaffi tzaffi Apr 13, 2022

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:

import pandas as pd

...
df = pd.DataFrame(...)

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):

import pyteal as pt
from pyteal import abi

toSum = abi.DynamicArray(abi.Uint64TypeSpec())
toMultiply = abi.DynamicArray(abi.Uint64TypeSpec())
results = abi.Tuple(abi.Uint64TypeSpec(), abi.Uint64TypeSpec())
program = pt.Seq(
    toSum.decode(pt.Txn.application_args[0]),
    toMultiply.decode(pt.Txn.application_args[1]),
    sumAndMultiply(toSum, toMultiply).store_into(results)
)

Copy link
Contributor

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:

  • If folks are on-board, I am on-board to use the pandas as pd approach. I think it'd help disambiguate PyTeal constructs that match Python standard library (e.g. Array).
  • If folks prefer to keep as is, I'd advocate to add the ignore only for tests and examples. I'm thinking in source code we can be more perspective. Though I'll ultimately follow the group's recommendation here.

Copy link
Contributor

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:

  • Use of star imports in our runtime code: this is highly useful because we have a few modules that import and export all of a submodule. It would be too time consuming and error prone to not use a star import here. My opinion is that adding inline exceptions for these cases is good enough.
  • Use of star imports in example: from the beginning we've encouraged this because PyTeal programs can use a lot of different AST elements, and because our syntax is already clunky enough without added prefixed in my opinion. I'm in favor of keeping it as-is since our examples should look natural and not be intimidating.
  • Use of star imports in our tests: this matters least to me, so if you'd like to adopt import pyteal as pt or similar, I wouldn't mind.

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'll go with the modifying the tests files only and adding flake8 exceptions for other files.

Copy link
Contributor Author

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

Copy link
Contributor

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.

ignore = E203, E221, E241, E266, W291, E302, F403, F405, E501, W503, E741
30 changes: 10 additions & 20 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,11 @@ on:
- v**
branches:
- master

jobs:
build-test:
runs-on: ubuntu-20.04
container: python:${{ matrix.python }}-slim
container: python:${{ matrix.python }}
strategy:
matrix:
python: ['3.10']
Expand All @@ -20,15 +21,10 @@ jobs:
with:
fetch-depth: 0
- name: Install pip dependencies
run: |
pip install -r requirements.txt
pip install -e .
run: make pip-development
- name: Build and Test
run: |
python scripts/generate_init.py --check
black --check .
mypy pyteal
pytest
run: make build-and-test

build-docset:
runs-on: ubuntu-20.04
container: python:3.10 # Needs `make`, can't be slim
Expand All @@ -38,21 +34,15 @@ jobs:
with:
fetch-depth: 0
- name: Install pip dependencies
run: |
pip install -r requirements.txt
pip install -r docs/requirements.txt
pip install doc2dash
run: make pip-docs
- name: Make docs
run: |
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
run: make docs
- name: Archive docset
uses: actions/upload-artifact@v2
with:
name: pyteal.docset
path: docs/pyteal.docset.tar.gz

upload-to-pypi:
runs-on: ubuntu-20.04
container: python:3.10
Expand All @@ -64,9 +54,9 @@ jobs:
with:
fetch-depth: 0
- name: Install dependencies
run: pip install wheel
run: make pip-wheel
- name: Build package
run: python setup.py sdist bdist_wheel
run: make bdist-wheel
- name: Release
uses: pypa/gh-action-pypi-publish@release/v1
with:
Expand Down
10 changes: 6 additions & 4 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
.DS_Store

# Byte-compiled / optimized / DLL files
__pycache__/
*.py[cod]
Expand Down Expand Up @@ -54,8 +52,8 @@ coverage.xml
.pytest_cache/

# Tests generating TEAL output to compared against an expected example.
tests/teal/*.teal
!tests/teal/*_expected.teal
tests/**/*.teal
!tests/**/*_expected.teal

# Translations
*.mo
Expand All @@ -76,6 +74,7 @@ instance/

# Sphinx documentation
docs/_build/
pyteal.docset*

# PyBuilder
target/
Expand Down Expand Up @@ -133,3 +132,6 @@ dmypy.json
# IDE
.idea
.vscode

# mac OS
.DS_Store
48 changes: 48 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
pip:
pip install -e .

pip-development: pip
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:
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

ALLPY = docs examples pyteal scripts tests *.py
black:
black --check $(ALLPY)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why $(ALLPY) and not .?

Copy link
Contributor Author

@tzaffi tzaffi Apr 14, 2022

Choose a reason for hiding this comment

The 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 (flake8 and eventually mypy). So I would prefer to keep this way.

Another approach is to specifically exclude certain folders with --exclude, but then everyone would have to configure that specifically for their local excludes.

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

test-unit:
pytest

build-and-test: build lint test-unit

# Extras:
coverage:
pytest --cov-report html --cov=pyteal

.PHONY: build docs
13 changes: 8 additions & 5 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,16 +51,19 @@ Active venv:
* `. venv/bin/activate.fish` (if your shell is fish)

Pip install PyTeal in editable state
* `pip install -e .`
* `make pip`

Install dependencies:
* `pip install -r requirements.txt`
* `make pip-development`

Type checking using mypy:
* `mypy pyteal`
* `make mypy`

Run tests:
* `pytest`
* `make test-unit`

Format code:
* `black .`
* `make black`

Lint using flake8:
* `make flake8`
1 change: 0 additions & 1 deletion examples/application/vote_deploy.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
# based off https://github.com/algorand/docs/blob/cdf11d48a4b1168752e6ccaf77c8b9e8e599713a/examples/smart_contracts/v2/python/stateful_smart_contracts.py

import base64
import datetime

from algosdk.future import transaction
from algosdk import account, mnemonic
Expand Down
10 changes: 7 additions & 3 deletions examples/signature/periodic_payment_deploy.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
#!/usr/bin/env python3

from pyteal import *
import uuid, params, base64
import base64
import params
import uuid

from algosdk import algod, transaction, account, mnemonic
from periodic_payment import periodic_payment, tmpl_amt
from periodic_payment import periodic_payment

from pyteal import *

# --------- compile & send transaction using Goal and Python SDK ----------

Expand Down
16 changes: 12 additions & 4 deletions examples/signature/recurring_swap_deploy.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,19 @@
#!/usr/bin/env python3

from pyteal import *
import base64
from nacl import encoding, hash
from recurring_swap import recurring_swap, tmpl_provider
from algosdk import algod, account
import params
import re
import time
import uuid

from algosdk import algod
from algosdk.future import transaction
import uuid, params, re, base64, time

from pyteal import *

from recurring_swap import recurring_swap


# ------- generate provider's account -----------------------------------------------
key_fn = str(uuid.uuid4()) + ".key"
Expand Down
4 changes: 2 additions & 2 deletions pyteal/__init__.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from .ast import *
from .ast import * # noqa: F401
from .ast import __all__ as ast_all
from .ir import *
from .ir import * # noqa: F401
from .ir import __all__ as ir_all
from .compiler import (
MAX_TEAL_VERSION,
Expand Down
4 changes: 2 additions & 2 deletions pyteal/__init__.pyi
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
## File generated from scripts/generate_init.py.
## DO NOT EDIT DIRECTLY

from .ast import *
from .ast import * # noqa: F401
from .ast import __all__ as ast_all
from .ir import *
from .ir import * # noqa: F401
from .ir import __all__ as ir_all
from .compiler import (
MAX_TEAL_VERSION,
Expand Down
5 changes: 0 additions & 5 deletions pyteal/ast/acct.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,8 @@
from typing import TYPE_CHECKING

from ..types import TealType, require_type
from ..ir import Op
from .expr import Expr
from .maybe import MaybeValue

if TYPE_CHECKING:
from ..compiler import CompileOptions


class AccountParam:
@classmethod
Expand Down
2 changes: 0 additions & 2 deletions pyteal/ast/acct_test.py
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
Expand Down
3 changes: 0 additions & 3 deletions pyteal/ast/asset.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
from enum import Enum

from ..types import TealType, require_type
from ..ir import Op
from .expr import Expr
from .leafexpr import LeafExpr
from .maybe import MaybeValue


Expand Down
2 changes: 0 additions & 2 deletions pyteal/ast/cond.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@
from ..ir import TealOp, Op, TealSimpleBlock, TealConditionalBlock
from ..errors import TealInputError
from .expr import Expr
from .err import Err
from .if_ import If

if TYPE_CHECKING:
from ..compiler import CompileOptions
Expand Down
2 changes: 0 additions & 2 deletions pyteal/ast/err_test.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import pytest

from .. import *


Expand Down
2 changes: 0 additions & 2 deletions pyteal/ast/for_.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@
from ..ir import TealSimpleBlock, TealConditionalBlock
from ..errors import TealCompileError
from .expr import Expr
from .seq import Seq
from .int import Int

if TYPE_CHECKING:
from ..compiler import CompileOptions
Expand Down
1 change: 0 additions & 1 deletion pyteal/ast/gitxn.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
from ..ir import TealOp, Op, TealBlock
from .expr import Expr
from .txn import TxnExpr, TxnField, TxnObject, TxnaExpr
from ..types import require_type, TealType

if TYPE_CHECKING:
from ..compiler import CompileOptions
Expand Down
2 changes: 1 addition & 1 deletion pyteal/ast/if_.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from typing import TYPE_CHECKING

from ..errors import TealCompileError, TealInputError
from ..types import TealType, require_type, types_match
from ..types import TealType, require_type
from ..ir import TealSimpleBlock, TealConditionalBlock
from .expr import Expr

Expand Down
2 changes: 1 addition & 1 deletion pyteal/ast/itxn_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
from ..types import types_match

# this is not necessary but mypy complains if it's not included
from .. import MAX_GROUP_SIZE, CompileOptions
from .. import CompileOptions

teal4Options = CompileOptions(version=4)
teal5Options = CompileOptions(version=5)
Expand Down
5 changes: 1 addition & 4 deletions pyteal/ast/maybe.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from typing import List, Union, TYPE_CHECKING
from typing import List, Union

from pyteal.ast.multi import MultiValue

Expand All @@ -7,9 +7,6 @@
from .expr import Expr
from .scratch import ScratchLoad, ScratchSlot

if TYPE_CHECKING:
from ..compiler import CompileOptions


class MaybeValue(MultiValue):
"""Represents a get operation returning a value that may not exist."""
Expand Down
2 changes: 0 additions & 2 deletions pyteal/ast/maybe_test.py
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
Expand Down
1 change: 0 additions & 1 deletion pyteal/ast/methodsig.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

from pyteal.types import TealType

from ..types import TealType
from ..ir import TealOp, Op, TealBlock
from .leafexpr import LeafExpr

Expand Down
10 changes: 7 additions & 3 deletions pyteal/ast/multi_test.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
from typing import List

from .. import *
from pyteal import *
from pyteal.ast import *
from pyteal.ir import *

options = CompileOptions()

Expand Down Expand Up @@ -168,15 +170,17 @@ def test_multi_value():
)
__test_single_conditional(expr, op, args, iargs, reducer)

reducer = lambda value, hasValue: Seq(Assert(hasValue), value)
reducer = lambda value, hasValue: Seq( # noqa: E731
Assert(hasValue), value
)
expr = MultiValue(
op, [type, TealType.uint64], immediate_args=iargs, args=args
)
__test_single_assert(expr, op, args, iargs, reducer)

hasValueVar = ScratchVar(TealType.uint64)
valueVar = ScratchVar(type)
reducer = lambda value, hasValue: Seq(
reducer = lambda value, hasValue: Seq( # noqa: E731
hasValueVar.store(hasValue), valueVar.store(value)
)
expr = MultiValue(
Expand Down
Loading