Skip to content

NF: distribution operations#116

Merged
yarikoptic merged 11 commits intoReproNim:masterfrom
chaselgrove:nf-distribution-operations
Oct 30, 2017
Merged

NF: distribution operations#116
yarikoptic merged 11 commits intoReproNim:masterfrom
chaselgrove:nf-distribution-operations

Conversation

@chaselgrove
Copy link
Contributor

Primitives for checking if Debian packages and distributions satisfy requirements in other packages and distributions.

@codecov-io
Copy link

codecov-io commented Oct 5, 2017

Codecov Report

Merging #116 into master will increase coverage by 0.04%.
The diff coverage is 80.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #116      +/-   ##
==========================================
+ Coverage   76.16%   76.21%   +0.04%     
==========================================
  Files         119      119              
  Lines        8244     8328      +84     
==========================================
+ Hits         6279     6347      +68     
- Misses       1965     1981      +16
Impacted Files Coverage Δ
niceman/distributions/tests/test_debian.py 98.07% <100%> (+2.33%) ⬆️
niceman/distributions/debian.py 88.67% <78.57%> (-1.24%) ⬇️
niceman/distributions/vcs.py 80% <0%> (-4.09%) ⬇️

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 d4bdac7...7ebf576. Read the comment docs.

Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

For now just minor whining from a Pythonista

for p in other.packages:
if not self.satisfies_package(p):
return False
return True
Copy link
Member

Choose a reason for hiding this comment

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

above four lines could be more pythonicly expressed

return all(map(self.satisfies_package, other.packages))

in python3 map is already a generator (so you would get early termination), in python2 -- not yet so it would go through all possibly for no reason. So just

from six.moves import map

for p in self.packages:
if p.satisfies(package):
return True
return False
Copy link
Member

Choose a reason for hiding this comment

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

see comment below ... eventually we might want to make it more efficient/avoid full list lookup every time.


def test_package_satisfies(setup_packages):
(p1, p1v10, p1v11, p2) = setup_packages
assert p1.satisfies(p1) is True
Copy link
Member

Choose a reason for hiding this comment

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

sufficient to

assert p1.satisfies(p1)
assert not p1.satisfies(p1v10)

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'd set it up this way so I could return None and make all tests fail to start with. You're right, at this point this idiom isn't needed any more.

@yarikoptic yarikoptic added the WiP label Oct 13, 2017
@yarikoptic
Copy link
Member

I added a WiP label. @chaselgrove please remove it when you think it is worth merging

@chaselgrove
Copy link
Contributor Author

If we can merge #121, I'd like to refactor this PR to take advantage of that code. Then let's merge this -- this could potentially be a WIP until all the classes are fleshed out (read: indefinitely :) ) but I'd rather do this iteratively.

@chaselgrove chaselgrove removed the WiP label Oct 27, 2017
@yarikoptic yarikoptic merged commit 11abdd2 into ReproNim:master Oct 30, 2017
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.

3 participants