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

Fix sdist package #4074

Merged
merged 2 commits into from
May 15, 2019
Merged

Fix sdist package #4074

merged 2 commits into from
May 15, 2019

Conversation

ulope
Copy link
Collaborator

@ulope ulope commented May 14, 2019

setuptools_scm forcibly includes all files under version control into the sdist package, ignoring MANIFEST.in and find_packages.
This fixes this by replacing the find_files function with a dummy one (see setup.py::EggInfo.__init__()).

See pypa/setuptools-scm#190

Additionally this uncovered that we had two directories that were missing __init__.py files (thereby being turned into namespace packages). That lead to mypy and pylint not covering them.
This is now also fixed here (including two bugs in migrations mypy found).

Setuptools_scm forcibly includes all files under version control into the sdist package, ignoring `MANIFEST.in` and `find_packages`.
This fixes this by replacing the `find_files` function with a dummy one (`see setup.py::EggInfo.__init__()`).

See pypa/setuptools-scm#190
raiden/storage/migrations/v18_to_v19.py Show resolved Hide resolved
@@ -4,15 +4,22 @@
from eth_utils import to_canonical_address
from gevent.pool import Pool

from raiden import raiden_service # pylint: disable=unused-import
Copy link
Contributor

Choose a reason for hiding this comment

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

can you please just import RaidenService class directly here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, this can be wrapped in if TYPE_CHECKING, import RaidenService since we're keeping the type "quoted" in the function declarations

@codecov
Copy link

codecov bot commented May 15, 2019

Codecov Report

Merging #4074 into master will decrease coverage by 1.53%.
The diff coverage is 76%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #4074      +/-   ##
=========================================
- Coverage   77.84%   76.3%   -1.54%     
=========================================
  Files         113     112       -1     
  Lines       15584   15568      -16     
  Branches     2177    2183       +6     
=========================================
- Hits        12131   11879     -252     
- Misses       2697    2932     +235     
- Partials      756     757       +1
Flag Coverage Δ
#fuzz 35.24% <44%> (?)
#integration 68.74% <44%> (?)
#unit 52.28% <76%> (?)
Impacted Files Coverage Δ
raiden/network/resolver/client.py 37.5% <50%> (+1.13%) ⬆️
raiden/storage/migrations/v19_to_v20.py 92.13% <66.66%> (-2.12%) ⬇️
raiden/storage/migrations/v17_to_v18.py 74.46% <75%> (-1.09%) ⬇️
raiden/storage/migrations/v18_to_v19.py 91.17% <80%> (ø) ⬆️
raiden/storage/migrations/v21_to_v22.py 83.03% <87.5%> (ø) ⬆️
raiden/storage/restore.py 46.66% <0%> (-40%) ⬇️
raiden/blockchain_events_handler.py 76.84% <0%> (-16.32%) ⬇️
raiden/network/rpc/smartcontract_proxy.py 68% <0%> (-12%) ⬇️
raiden/raiden_event_handler.py 82.04% <0%> (-7.76%) ⬇️
raiden/network/proxies/token_network.py 66.53% <0%> (-6.38%) ⬇️
... and 40 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6cc35dc...e6afb13. Read the comment docs.

@@ -175,10 +189,10 @@ def _add_onchain_locksroot_to_snapshot(
channel["our_state"]["onchain_locksroot"] = serialize_bytes(our_locksroot)
channel["partner_state"]["onchain_locksroot"] = serialize_bytes(partner_locksroot)

return json.dumps(snapshot, indent=4), snapshot_record.identifier
return json.dumps(snapshot, indent=4), snapshot_record.state_change_identifier
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is what we want here: snapshot_record.identifier should/does exist and is the unique id for the affected snapshot. Why did you change it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because snapshot_record is a StateChangeRecord (at least according to the function annotation) and therefore mypy (and PyCharm) complained.

Copy link
Contributor

@konradkonrad konradkonrad May 15, 2019

Choose a reason for hiding this comment

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

yeah, the typing of the signature is incorrect then: https:/raiden-network/raiden/pull/4074/files#diff-30896e13fdd21557ed5512f90093e729R196

Should be a

class SnapshotRecord(NamedTuple):

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok makes sense. I changed it.

Copy link
Contributor

@konradkonrad konradkonrad left a comment

Choose a reason for hiding this comment

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

looks good, thank you @ulope!
btw: this resolves the drive-by issue raised here

@ulope ulope merged commit aa66a4e into raiden-network:master May 15, 2019
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.

4 participants