Review filter lists and compare_entities
Summary
Contains all changes from !151 (closed) and !137 (merged) as well as additional tests and some fixes.
Focus
- Changes from the current version of compare are in this diff
Test Environment
Manual Tests
Check List for the Author
-
All automated tests pass -
Reference related issuesMRs -
Up-to-date CHANGELOG.md (or not necessary) -
Appropriate user and developer documentation (or not necessary) -
Annotations in code (Gitlab comments)
Check List for the Reviewer
-
I understand the intent of this MR -
All automated tests pass -
Up-to-date CHANGELOG.md (or not necessary) -
Appropriate user and developer documentation (or not necessary) -
The test environment setup works and the intended behavior is reproducible in the test environment -
In-code documentation and comments are up-to-date. -
Check: Are there specifications? Are they satisfied?
Merge request reports
Activity
requested review from @henrik
assigned to @i.nueske
- Resolved by Henrik tom Wörden
114 114 IMPORTANCE = Literal["OBLIGATORY", "RECOMMENDED", "SUGGESTED", "FIX", "NONE"] 115 115 ROLE = Literal["Entity", "Record", "RecordType", "Property", "File"] 116 116 117 SPECIAL_ATTRIBUTES = ["name", "role", "datatype", "description", 118 "id", "path", "checksum", "size", "value"] 117 SPECIAL_ATTRIBUTES = ["name", "role", "datatype", "description", "file", 118 "id", "path", "checksum", "size", "value", "unit"] - Comment on lines -117 to +118
I also added 'file' to the SPECIAL_ATTRIBUTES as added files did not show up as a difference in the diff otherwise. If I understood the documentation correctly it should be in there anyway and all unit tests still succeed, but I am not sure that this will not have other repercussions, so someone more familiar with the code should recheck
225 if type(old_entity) is not type(new_entity): 241 # ToDo: Discuss intended behaviour 242 # Questions that need clarification: 243 # - What is intended behaviour for multi-properties and multi-parents? 244 # - Do different inheritance levels for parents count as a difference? 245 # - Do we care about parents and properties of properties? 246 # - Should there be a more detailed comparison of parents without id? 247 # - Revisit filter - do we care about RecordType when matching? 248 # How to treat None? 249 # Suggestions for enhancements: 250 # - For the comparison of entities in value and properties, consider 251 # keeping a list of traversed entities, not only look at first layer 252 # - Make the empty_diff functionality faster by adding a parameter to 253 # this function so that it returns after the first found difference? 254 # - Add parameter to restrict diff to some characteristics 255 entity0, entity1 = old_entity, new_entity In the code I would suggest using entity0 and entity1 as discussed, because old and new usually indicate a changed value and are somewhat confusing here. We did not reach a conclusion in our discussion though iirc, so you'd need to look at this again and decide whther it does improve readability.
changed this line in version 4 of the diff
- Resolved by Henrik tom Wörden
- Resolved by Henrik tom Wörden
- Resolved by Henrik tom Wörden
379 # We do not care about parents and properties here, discard 380 od.pop("parents") 381 od.pop("properties") 382 nd.pop("parents") 383 nd.pop("properties") 384 # use the remaining diff 385 propdiff[0].update(od) 386 propdiff[1].update(nd) 387 388 # As the importance of a property is an attribute of the record 389 # and not the property, it is not contained in the diff returned 390 # by compare_entities and needs to be added separately 391 if (entity0.get_importance(prop) != 392 entity1.get_importance(matching[0])): 393 propdiff[0]["importance"] = entity0.get_importance(prop) 394 propdiff[1]["importance"] = entity1.get_importance(matching[0]) - Resolved by Henrik tom Wörden
170 166 assert diff_r2["properties"]["test"]["unit"] == "m" 171 167 172 168 169 def test_compare_entities_battery(): 170 par1, par2, par3 = db.Record(), db.Record(), db.RecordType() 171 r1, r2, r3 = db.Record(), db.Record(), db.Record() 172 prop1 = db.Property() 173 prop2 = db.Property(name="Property 2") 174 prop3 = db.Property() mentioned in issue #205 (closed)
5552 potentials = list(zip(listobject.copy(), [False]*len(listobject))) 5553 for candidate, wrapped_is_checked in potentials: 5554 name_match, pid_match, original_candidate = False, False, None 5555 5556 # Parents/Properties may be wrapped - if wanted, try to match original 5557 # Note: if we want to check all wrapped Entities, this should be 5558 # switched. First check the wrap, then append wrapped. In this 5559 # case we also don't need wrapped_checked, but preferentially 5560 # append the wrapper. 5561 if check_wrapped and not wrapped_is_checked: 5562 try: 5563 if candidate._wrapped_entity is not None: 5564 original_candidate = candidate 5565 candidate = candidate._wrapped_entity 5566 except: 5567 pass added 2 commits
added 1 commit
- cb9aba79 - DEPR: deprecate old_entity/new_entity of compare_entities
enabled an automatic merge when all merge checks for cb9aba79 pass
enabled an automatic merge when all merge checks for 133c0e36 pass
mentioned in commit 649071f3
mentioned in merge request !157 (merged)
mentioned in merge request caosdb-advanced-user-tools!115 (merged)