Conversation
Codecov Report
@@ Coverage Diff @@
## master #121 +/- ##
==========================================
- Coverage 72.49% 72.33% -0.16%
==========================================
Files 116 116
Lines 7791 7791
==========================================
- Hits 5648 5636 -12
- Misses 2143 2155 +12
Continue to review full report at Codecov.
|
yarikoptic
left a comment
yarikoptic
left a comment
There was a problem hiding this comment.
we were dreaming of doing that awhile back:
https://github.com/ReproNim/niceman/blob/a507746847a1466c4c4ac1e64197d77f5250ca81/niceman/distributions/debian.py#L73
but decided not to bother "for now", so may be it is time. I am not certain though that we wouldn't at some point need to introduce changes to existing ones, where copy wouldn't be way to go... but I guess until then, it is all ok.
I guess the tests in whatever you need sets of packages for would excercise this fact (immutability) so would fail if we happen to revert "by mistake" ;)
niceman/distributions/debian.py
Outdated
| return None | ||
|
|
||
| # prep our pkg object: | ||
| # prep our pkg object: |
niceman/distributions/debian.py
Outdated
| source_version = attr.ib(default=None) | ||
| versions = attr.ib(default=None) | ||
| install_date = attr.ib(default=None) | ||
| version = attr.ib(default=None, hash=True) |
There was a problem hiding this comment.
it is hard(er) to differentiate between those with True or False. The default for hash seems to be True, so I would prefer if , hash=True ones were removed, then it would be easier to see which fields we consider not important
niceman/distributions/debian.py
Outdated
| versions = attr.ib(default=None) | ||
| install_date = attr.ib(default=None) | ||
| version = attr.ib(default=None, hash=True) | ||
| architecture = attr.ib(default=None, hash=False) |
There was a problem hiding this comment.
there could be multiple instances of the package with the same name and version but different architecture
$> dpkg -l zlib1g:*
Desired=Unknown/Install/Remove/Purge/Hold
| Status=Not/Inst/Conf-files/Unpacked/halF-conf/Half-inst/trig-aWait/Trig-pend
|/ Err?=(none)/Reinst-required (Status,Err: uppercase=bad)
||/ Name Version Architecture Description
+++-====================-===============-===============-==============================================
ii zlib1g:amd64 1:1.2.8.dfsg-5 amd64 compression library - runtime
ii zlib1g:i386 1:1.2.8.dfsg-5 i386 compression library - runtimeso should participate in the hashing
|
Also note that you would conflict in changes with #115, so might be better to wait for that one to get merged first (before working out other classes etc) |
|
I merged #115 so this can proceed (though I will make a new pull request to resolve some review suggestions). GitHub claims that there are still no conflicts in this branch... |
|
(I spoke too soon... as soon as I submitted the comment github noted some conflicts in this branch... sorry) |
|
Updated in response to comments. |
|
conflicts should be resolved before itcould be considered for merging |
|
@chaselgrove ping |
|
@chaselgrove ping ping |
|
Fixed conflicts. |
4 participants
This PR makes DEBPackage immutable so they are hashable (and usable in sets -- my ultimate motivation :) ).