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