-
Notifications
You must be signed in to change notification settings - Fork 114
Conversation
@The-42
The-42
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea, but I'm a bit divided about whether we should call it "check". This verb does not state what is checked and could easily misinterpreted as "check if there are overlaps, then explode".
How about "identical" instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please amend the method's documentation a couple lines higher as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that identical is clearer than check (even if it's not a verb as for other methods).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test in itself looks legit, but the test_merge_wrong_args right above this one now yields an incomplete message if triggered. Please add the new argument into that message and make it work again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum, seems that I didn't play the test... Any it's fixed.
I also updated the docs/
311e9b3 to
855fe8b
Compare
MarcFinetRtone
commented
Dec 30, 2020
v2:
- replace 'check' with 'identical'
- fix test
- fix doc
When merging multiple .hex files, in case of overlap we sometimes need to just 'ensure' that values are identical. The new 'identical' mode ensures this. Note: the behavior for start_addr is currently a 'identical' (i.e. the overlap configuration is checked only when values differ). So we might want to change the behavior for data too: - if data is identical: silently bail out - add a 'stricter' mode that corresponds to the current behavior
855fe8b to
5e2b05b
Compare
When merging multiple .hex files, in case of overlap we sometimes need to
just 'ensure' that values are identical.
The new 'check' mode ensures this.
Note: the behavior for start_addr is currently a 'check' (i.e. the
overlap configuration is checked only when values differ). So we might
want to change the behavior for data too: