Skip to content
Snippets Groups Projects

Review filter lists and compare_entities

Merged I. Nüske requested to merge f-review-lists-and-compare into dev
5 unresolved threads

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 issues MRs
  • 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?
Edited by I. Nüske

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • 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
      Author Developer

      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

    • Please register or sign in to reply
  • I. Nüske added 1 commit

    added 1 commit

    • c746e377 - MNT: Hide namechange from API

    Compare with previous version

  • 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
  • I. Nüske
  • I. Nüske
  • I. Nüske
  • 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])
    • Comment on lines +388 to +394
      Author Developer

      I changed the get_importance call from name to the property entity, as this should also work for entities that do not have a name, just an id, while still working for entities that have a name but no id.

    • Please register or sign in to reply
  • I. Nüske
  • 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

    Compare with previous version

  • added 1 commit

    • cb9aba79 - DEPR: deprecate old_entity/new_entity of compare_entities

    Compare with previous version

  • Henrik tom Wörden enabled an automatic merge when all merge checks for cb9aba79 pass

    enabled an automatic merge when all merge checks for cb9aba79 pass

  • Henrik tom Wörden canceled the automatic merge

    canceled the automatic merge

  • added 1 commit

    Compare with previous version

  • Henrik tom Wörden enabled an automatic merge when all merge checks for 133c0e36 pass

    enabled an automatic merge when all merge checks for 133c0e36 pass

  • mentioned in commit 649071f3

  • Henrik tom Wörden mentioned in merge request !157 (merged)

    mentioned in merge request !157 (merged)

  • Please register or sign in to reply
    Loading