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

Make the project packageable #59

Merged
merged 14 commits into from
Jan 24, 2020
Merged

Make the project packageable #59

merged 14 commits into from
Jan 24, 2020

Conversation

teknico
Copy link
Collaborator

@teknico teknico commented Jan 16, 2020

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:

  • rename the pydecred subpackage to dcr
  • use relative imports where appropriate
  • create the decred package and move the library code and tests there
  • complete the decred package definition and make it installable
  • create the tinywallet package and move the wallet app code and tests there
  • complete the tinywallet package definition and make it installable
  • update the Github Action definition file
  • update the documentation and examples

@teknico teknico mentioned this pull request Jan 16, 2020
@teknico teknico requested review from buck54321 and removed request for buck54321 January 16, 2020 14:01
@rodrigondec
Copy link

@teknico include a fixes #34 too

@teknico
Copy link
Collaborator Author

teknico commented Jan 16, 2020

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

@buck54321
Copy link
Member

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

@buck54321
Copy link
Member

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
Copy link
Collaborator Author

teknico commented Jan 16, 2020

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
Copy link
Member

Yeah, that's even better.

tinywallet/tinywallet/app.py Outdated Show resolved Hide resolved
tinywallet/tinywallet/app.py Outdated Show resolved Hide resolved
- create the tinywallet package and move the wallet code there;
- create the tinywallet pyproject.toml configuration file;
- create and commit the poetry.lock file (as recommended for apps);
- create a tinywallet.app.main() entry point and use it for the app script;
- change tinywallet.app.cfg from a global to a parameter.
- reverted a change to the blockchain db filename;
- moved the config initialization to the TinyDecred class constructor;
- moved the log initialization to the TinyDecred class constructor;
- turned the tryExecute function into a method of the TinyDecred class,
  so that it can use logs without having to pass them as a parameter.
@teknico
Copy link
Collaborator Author

teknico commented Jan 22, 2020

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 teknico mentioned this pull request Jan 22, 2020
4 tasks
- Reword the top README file to only describe the project, moving details
  to other README files;
- move the decred/decred/dcr/README.md file to decred/README.md, adding
  the relevant details from the top one;
- create a tinywallet/README.md file, adding the relevant details from
  the top one;
- create a CONTRIBUTING.md file with contribution guidelines.
@teknico teknico marked this pull request as ready for review January 22, 2020 17:18
@teknico
Copy link
Collaborator Author

teknico commented Jan 22, 2020

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

README.md Outdated Show resolved Hide resolved
decred/decred/crypto/crypto.py Show resolved Hide resolved
decred/pyproject.toml Show resolved Hide resolved
tinywallet/pyproject.toml Show resolved Hide resolved
Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

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

Looking good.

  • Should tinywallet get its own .coveragerc?

  • I ran into an issue that other probably won't, but maybe I can save someone some time. poetry was failing for me with a complaint about a missing bz2 module. Since I had built Python from source and didn't have the library, the bz2 module was never installed. On Debian, I had to 1) get the right library with apt-get install libbz2-dev, 2) rebuild Python, and 3) reinstall poetry.

  • poetry install went fine from decred, but has issues with tinywallet which required me to set a specific PyQt5 version. There were a few other issues too. Are you testing tinywallet too? My issues may be system-specific, though.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
decred/README.md Outdated Show resolved Hide resolved
decred/README.md Outdated Show resolved Hide resolved
tinywallet/pyproject.toml Outdated Show resolved Hide resolved
examples/testnet
decred/dist
tinywallet/dist
decred/poetry.lock
Copy link
Member

Choose a reason for hiding this comment

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

I see you're not ignoring the tinywallet/poetry.lock. Is that intentional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is, well spotted. tinywallet/poetry.lock is indeed committed, decred/poetry.lock is not. The idea is to commit lock files for applications, but not for libraries.

You want to commit the lock file for apps so that a build always uses the same versions of the dependencies (and their dependencies), and therefore is reproducible.

You don't want to commit the lock file for libraries because then you would have multiple lock files for different libraries and for the app that uses them, potentially conflicting among them.

The libraries will specify the version intervals for their dependencies in their pyproject.toml or requirements.txt files; Poetry will respect those intervals, but will disregard any libraries' lock files. The only lock file that counts is the app one.

Copy link
Member

Choose a reason for hiding this comment

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

This is probably nothing, but using the provided tinywallet/poetry.lock file threw an error at me:

[joe@hyrule tinywallet]$ poetry install
Installing dependencies from lock file
Warning: The lock file is not up to date with the latest changes in pyproject.toml. You may be getting outdated dependencies. Run update to update them.

[SolverProblemError]
Because tinywallet depends on pyqt5 (=5.12.3) which doesn't match any versions, version solving failed.

Running poetry update solved the problem, but it gave me a rather large diff.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, forgot to update the dependencies and the lock file. Thanks.

tinywallet/tinywallet/app.py Outdated Show resolved Hide resolved
@teknico
Copy link
Collaborator Author

teknico commented Jan 23, 2020

Should tinywallet get its own .coveragerc?

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

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

- added the Python versions 3.5, 3.6 and 3.8 to the action definition file;
- reworded the various README.md and CONTRIBUTING.md files;
- updated the PyQt5 dependency version for tinywallet;
- reverted a blockchain db filename change in tinywallet's app.py
  (for real this time).
@teknico
Copy link
Collaborator Author

teknico commented Jan 23, 2020

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
Copy link
Member

JoeGruffins commented Jan 24, 2020

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

Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

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

Caught one more thing while publishing to test.pypi.

https://test.pypi.org/project/decred/

decred/pyproject.toml Outdated Show resolved Hide resolved
decred/pyproject.toml Outdated Show resolved Hide resolved
@buck54321
Copy link
Member

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.

@buck54321 buck54321 merged commit 4409d7d into decred:master Jan 24, 2020
@teknico teknico deleted the make-packages branch January 25, 2020 08:48
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.

Packaging tinydecred tinydecred as a toolkit
4 participants