Conversation
| 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)) |
| 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?
colesmj
left a comment
colesmj
left a comment
There was a problem hiding this comment.
I think the change made - to change the name of the dummy finding on pytm.py:670 for easier filtering later is reasonable, but I think some more conversation is needed around Finding.element vs .target and whether .element needs to be required.
| 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.
| 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.
| 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?
|
Trying for some PR cleanup. |
|
Don't know, I'll test when I get home later today. |
3 participants
Watching out for the "invalid" Element created when a Finding is added by hand.