RF: satisfies, diff and alike #273
Conversation
niceman/distributions/base.py
Outdated
|
|
||
| @property | ||
| def _cmp_id(self): | ||
| if not self._cmp_fields: |
There was a problem hiding this comment.
Hmm, but an empty tuple would go down this path. That seems to contradict above which says that an empty tuple means "all attribs establish identity".
There was a problem hiding this comment.
I'm not sure I like the semantics of an empty _cmp_fields meaning use all of the attributes. I'd rather require classes to define _cmp_fields explicitly and let the error be raised if they aren't. This will mean one less special case we have to keep track of ("nothing means everything") and one more defense against a forgetful coder not thinking about _cmp_fields in a new class.
There was a problem hiding this comment.
@chaselgrove Sounds reasonable to me. I've updated the comment to resolve the comment/code mismatch.
There was a problem hiding this comment.
Thanks. Also removed the fallback to all attributes in the code.
There was a problem hiding this comment.
Ah, thanks, I hadn't spotted that. So there was originally a code/comment mismatch and a code/code mismatch (the fallback was inaccessible due to the not self._cmp_fields guard).
niceman/distributions/base.py
Outdated
| # Might need to be gone or some custom exception | ||
| raise RuntimeError( | ||
| "Cannot establish identity of %r since _cmp_fields " | ||
| "are not defined", self) |
There was a problem hiding this comment.
This message won't be formatted for you.
niceman/distributions/base.py
Outdated
| "are not defined", self) | ||
| fields = self._cmp_fields \ | ||
| if self._cmp_fields \ | ||
| else self._attr_names |
There was a problem hiding this comment.
or fields = self._cmp_fields or self._attr_names
niceman/distributions/base.py
Outdated
|
|
||
| # TODO: code here is too generic and could be used for "is_identical_to" | ||
| # kind of check, just logic could be adjusted | ||
| def is_satisfied_by(self, other): |
There was a problem hiding this comment.
I like the name is_statisfied_by, though I think dropping the "is" (satisfied_by) would be better.
niceman/distributions/base.py
Outdated
| # TODO: code here is too generic and could be used for "is_identical_to" | ||
| # kind of check, just logic could be adjusted | ||
| def is_satisfied_by(self, other): | ||
| """does the other spec object satisfies the requirements of the current one |
There was a problem hiding this comment.
s/satisfies/satisfy/
Stylistically, I'd prefer
Does the spec object `other` satisfy the requirements of this one?
niceman/distributions/vcs.py
Outdated
| # # (to the caller) that identifier is not defined | ||
| # msg = "%s instance has no attribute 'identifier'" % self.__class__ | ||
| # raise AttributeError(msg) | ||
| # |
There was a problem hiding this comment.
IMO there's no reason to leave commented out code unless it comes with a TODO comment.
There was a problem hiding this comment.
A comment later in the code referred to the block. :)
Gone.
niceman/interface/diff.py
Outdated
| if s.endswith('repository'): | ||
| return s.replace('repository', 'repositories') | ||
| else: | ||
| return s + 's' |
There was a problem hiding this comment.
This is currently only used in one place. Do you plan to use it elsewhere? If not it doesn't seem worth having a separate function.
niceman/interface/diff.py
Outdated
|
|
||
| dist_1 = env_1.get_distribution(dist_type) | ||
| if dist_1: | ||
| pkgs_1 = { p._cmp_id: p for p in dist_1.packages } | ||
| pkgs_1 = {p._cmp_id: p for p in dist_1.packages} |
There was a problem hiding this comment.
It'd be better if you'd do purely cosmetic changes like this and the one below in a separate commit.
niceman/interface/diff.py
Outdated
| if pkgs_only_1 or pkgs_only_2: | ||
| print(pkg_type + 's:') | ||
| print(_make_plural(pkg_type) + ':') | ||
|
|
There was a problem hiding this comment.
When I run diff on a spec with Git repos, it fails because the code below assumes a version attribute.
There was a problem hiding this comment.
There were a number of tests that were failing; I've fixed these.
niceman/interface/diff.py
Outdated
| # print('%s pkgsitory %s:' % (repo_type, repo_id)) | ||
| # print('< %s (%s)' % (repo_1.commit, repo_1.path)) | ||
| # print('---') | ||
| # print('> %s (%s)' % (repo_2.commit, repo_2.path)) |
There was a problem hiding this comment.
Is there any reason to leave this code commented out rather than deleting it?
There was a problem hiding this comment.
Probably left over to refer to during refactoring. Gone now.
Codecov Report
@@ Coverage Diff @@
## master #273 +/- ##
==========================================
+ Coverage 89.27% 89.28% +0.01%
==========================================
Files 137 135 -2
Lines 10103 9853 -250
==========================================
- Hits 9019 8797 -222
+ Misses 1084 1056 -28
Continue to review full report at Codecov.
|
The code raises an error if _cmp_fields is empty. Re: ReproNim#273 (comment)
niceman/distributions/base.py
Outdated
| continue | ||
| if self_value != other_value: | ||
| return False | ||
| return True |
There was a problem hiding this comment.
snippet accompanying removed code if you decide to roll back
$> git diff
diff --git a/niceman/distributions/base.py b/niceman/distributions/base.py
index 13f6f9d..7dbb07c 100644
--- a/niceman/distributions/base.py
+++ b/niceman/distributions/base.py
@@ -156,6 +156,11 @@ class SpecObject(object):
if isinstance(self_v, SpecObject):
if not self_v.satisfied_by(other_v):
return differ("the other does not satisfy self")
+ elif isinstance(self_v, TypedList):
+ return all(
+ any(self_v_e.satisfied_by(o) for o in other_v)
+ for self_v_e in self_v
+ )
else:
# For now (and otherwise) - just a simple identity check
if self_v != other_v:
|
seems, minor python3 issues, e.g. and wonder what is your feel @chaselgrove about merging current state into master for the rename (#359) |
Codecov Report
@@ Coverage Diff @@
## master #273 +/- ##
=========================================
Coverage ? 88.79%
=========================================
Files ? 137
Lines ? 10217
Branches ? 0
=========================================
Hits ? 9072
Misses ? 1145
Partials ? 0
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #273 +/- ##
==========================================
+ Coverage 89.81% 89.83% +0.01%
==========================================
Files 148 148
Lines 11669 11738 +69
==========================================
+ Hits 10481 10545 +64
- Misses 1188 1193 +5
Continue to review full report at Codecov.
|
Fixed by a commit I hadn't pushed.
Sure, let's merge and reopen the other points as needed. |
|
Please try the diff on these two traces http://www.onerussian.com/tmp/niceman-traces-1.tgz which were generated I believe from two installations of niceman, one was done with just '.' and another one '.[full]' |
|
one of the development "use cases" is to use core functionality here to provide comparison/matching of the Distributions and Packages for the purpose of fixing those duplicates. chicken-and-egg problem of a kind |
…NDistribution, and VenvDistribution
…s, not instance (which may be None)
changed diff to a_and_b
collecting diff objects in a_only and b_only
|
fresh issue happens with the absence of unassociated files |
The idea in this PR to make comparison (diff) and satisfaction check logic to be implemented in a generic way, so only that semantic or custom handling would be assigned to the corresponding subclasses of SpecObject or their attributes.
SpecObject.is_satisfied_byhas replaced the customDebianPackage.satisfieswithout breaking any testsDebianDistribution.satisfiesso it could be replaced by the genericis_satisfied_by, test that previous/tested functionality worksis_satisfied_byinto a more genericcomparemethod with amode(or other name) kwarg with possible values of:satisfied_by,identical_to(anything else? better names? later on we could come up with e.g.upgraded_toto check if a given env is an upgraded version of an existing one)TypedListto also provide thecompare, wheresatisfied_bywould succeed if providedotheris a subset where each entrysatisfied_by, and then identical set withidentical_tofiles = attrib(default=attr.Factory(list))into a dedicatedfiles = FileList()which would probably be just an instance of aTypedList(str)to provide needed semantics for comparison, but better to have a dedicatedFileListadapter happen we need to provide anything custom beyondTypedListdiffthen could benefit from all the above to be generic:diffto provide bothidentical_toandsatisfied_bysemantic-
identical_toit is a regular "all are identical to each other"-
satisfied_byis an incremental satisfaction -- "each next one satisfies all previous ones"Whatever is not done within this PR should be moved into separate issues/PRs