Issue #222: fixed Findings creating spuriois Elements #223
Conversation
| class varControls(var): | ||
| def __set__(self, instance, value): | ||
| if not isinstance(value, Controls): | ||
| raise ValueError( | ||
| "expecting an Controls " | ||
| "value, got a {}".format(type(value)) | ||
| "expecting an Controls " "value, got a {}".format(type(value)) |
There was a problem hiding this comment.
| "expecting an Controls " "value, got a {}".format(type(value)) | |
| "expecting an Controls value, got a {}".format(type(value)) |
| @@ -661,7 +667,7 @@ def __init__( | |||
| if args: | |||
| element = args[0] | |||
| else: | |||
| element = kwargs.pop("element", Element("invalid")) | |||
| element = kwargs.pop("element", Element("created from Finding")) | |||
There was a problem hiding this comment.
Maybe the Element() is used here as a default value only to avoid checking later if element is None. If that's the case, it's better to explicitly check for it in this function, than check for a reserved name elsewhere in the code.
There was a problem hiding this comment.
In class Finding, element is set to required. If Finding can be connected to the tm rather than a specific Element, it seems this might address the problem. A dummy element is being created just to satisfy this constraint.
There was a problem hiding this comment.
Furthermore, Finding also has 2 attributes on associating with Elements:
- element - required Element object
- target - optional - string?
Maybe we need to remove one?
| if hasattr(e.source, "isEncryptedAtRest"): | ||
| for d in e.data: | ||
| d._safeset( | ||
| "isSourceEncryptedAtRest", e.source.controls.isEncryptedAtRest | ||
| "isSourceEncryptedAtRest", | ||
| e.source.controls.isEncryptedAtRest, |
There was a problem hiding this comment.
This change appears to go against the convention you have for other changes in this set - from a single line _safeset() to 2-line, but here you have 3-lines. Try to be consistent.
| @@ -661,7 +667,7 @@ def __init__( | |||
| if args: | |||
| element = args[0] | |||
| else: | |||
| element = kwargs.pop("element", Element("invalid")) | |||
| element = kwargs.pop("element", Element("created from Finding")) | |||
There was a problem hiding this comment.
In class Finding, element is set to required. If Finding can be connected to the tm rather than a specific Element, it seems this might address the problem. A dummy element is being created just to satisfy this constraint.
| @@ -661,7 +667,7 @@ def __init__( | |||
| if args: | |||
| element = args[0] | |||
| else: | |||
| element = kwargs.pop("element", Element("invalid")) | |||
| element = kwargs.pop("element", Element("created from Finding")) | |||
There was a problem hiding this comment.
Furthermore, Finding also has 2 attributes on associating with Elements:
- element - required Element object
- target - optional - string?
Maybe we need to remove one?
Watching out for the "invalid" Element created when a Finding is added by hand.