decred / tinydecred

Python tools for Decred
ISC License
27 stars 14 forks source link

Make the project packageable #59

Closed teknico closed 4 years ago

teknico commented 4 years ago

Fixes #34. Fixes #35 (and partially #32 too).

This restructures the project to make it packageable and installable from the Package Index.

It splits the project into two packages: the decred library and the tinywallet application, the latter depending on the former. It includes the following changes:

rodrigondec commented 4 years ago

@teknico include a fixes #34 too

teknico commented 4 years ago

@rodrigondec, is something in #34 missing here? What specifically?

buck54321 commented 4 years ago

@teknico I think @rodrigondec is suggesting that you mention issue #34 in your PR description.

buck54321 commented 4 years ago

PEP 8 generally recommends absolute imports, except in the case of complex package structures where absolute imports are too verbose.

Absolute imports are recommended, as they are usually more readable and tend to be better behaved (or at least give better error messages)...

However, explicit relative imports are an acceptable alternative to absolute imports, especially when dealing with complex package layouts where using absolute imports would be unnecessarily verbose

So let's have absolute imports as our default, with relative imports acceptable if the import statement is three levels or deeper. So

from tinydecred.pydecred.dcrdata import DcrdataBlockchain

can be shortened to

from .dcrdata import DcrdataBlockchain

but leave imports like

from tinydecred.util import encode

as is.

Is that fair?

teknico commented 4 years ago

How about a different approach?

The file decred/crypto/secp256k1/curve.py contains the following lines:

from decred.util.encode import ByteArray
from decred.crypto.rando import generateSeed
from decred.crypto.secp256k1.field import BytePoints, FieldVal

As relative imports they would become:

from ...util.encode import ByteArray
from ..rando import generateSeed
from .field import BytePoints, FieldVal

With the three-level rule all three lines would end up as relative, but only the third line is clearer as a relative import: the first one is clearer as absolute. The second line is less evident, but I'd go with absolute there too.

Therefore I'd like to suggest a different rule: only use relative imports for modules in the same subpackage or below, that is, only one dot allowed, never go up in the hierarchy.

(As an aside, the pycodestyle tool that implements the PEP8 guidelines has no checks for relative imports.)

buck54321 commented 4 years ago

Yeah, that's even better.

teknico commented 4 years ago

Mmm... I overhauled the action definition file to use Poetry and it suspiciously worked on the first try, the logs look fine. Oh well...

teknico commented 4 years ago

Ready for final review. You can check the live docs on the proposed branch.

teknico commented 4 years ago

Should tinywallet get its own .coveragerc?

It should. It should also get some tests. Until then, the coverage is probably going to be zero. :slightly_smiling_face:

poetry install went fine from decred, but has issues with tinywallet which required me to set a specific PyQt5 version.

Interesting, it seemed to work for me with 5.9.2. I will change it to 5.12.3 as mentioned above.

There were a few other issues too. Are you testing tinywallet too?

I am. The only problem I had was the Poetry relative path bug, and I worked around it using an absolute path (that I'm not committing, of course).

teknico commented 4 years ago

Black requires at least Python 3.6, so I prevented it from running under 3.5 in the PR check.

Next, we're using formatted string literals in nine places and they've been introduced in Python 3.6, so pyflakes raises a SyntaxError under 3.5.

I'll remove them from the codebase, and then we'll discover the next feature unsupported by 3.5.

EDIT: removing support for Python 3.5 instead.

JoeGruffins commented 4 years ago

I'm new to poetry buy after reading a little bit tinywallet is running without problems for me. python was installed via my package manager and poetry with pip3

buck54321 commented 4 years ago

Noting here that publishing with Poetry

poetry config repositories.testpypi https://test.pypi.org/simple
poetry publish -r testpypi

was failing silently.

To see my error, I had to

poetry run twine upload --repository-url https://test.pypi.org/legacy/ dist/*

which is sort of roundabout. This appears to be a known issue, so hopefully only a temporary problem. Poetry is still going through some growing pains, apparently, but the goods outweigh the bads for sure.