GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — merge: support 'identical' on overlapped data by MarcFinetRtone · Pull Request #44 · python-intelhex/intelhex

Conversation

@MarcFinetRtone

@MarcFinetRtone MarcFinetRtone commented Jul 9, 2020

Copy link
Copy Markdown

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:

  • if data is identical: silently bail out
  • add a 'stricter' mode that corresponds to the current behavior

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@The-42 The-42 self-assigned this Oct 25, 2020
@The-42 The-42 added the enhancement label Oct 25, 2020
The-42
The-42 requested changes Oct 27, 2020
View reviewed changes

@The-42 The-42 left a comment

Copy link
Copy Markdown
Collaborator

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread intelhex/__init__.py Outdated
if other is self:
raise ValueError("Can't merge itself")
if overlap not in ('error', 'ignore', 'replace'):
if overlap not in ('error', 'ignore', 'replace', 'check'):

@The-42 The-42 Oct 27, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@MarcFinetRtone MarcFinetRtone Dec 30, 2020

Copy link
Copy Markdown
Author

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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread intelhex/test.py
self.assertRaisesMsg(intelhex.AddressOverlapError,
'Data at address 0x0 is different: 0x1 vs. 0x2',
ih1.merge, ih2, overlap='check')

@The-42 The-42 Oct 27, 2020

Copy link
Copy Markdown
Collaborator

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@MarcFinetRtone MarcFinetRtone Dec 30, 2020

Copy link
Copy Markdown
Author

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/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@MarcFinetRtone
MarcFinetRtone force-pushed the new-check-for-overlapping-data branch 2 times, most recently from 311e9b3 to 855fe8b Compare December 30, 2020 15:07
@MarcFinetRtone MarcFinetRtone changed the title merge: support 'check' on overlapped data merge: support 'identical' on overlapped data Dec 30, 2020
@MarcFinetRtone

MarcFinetRtone commented Dec 30, 2020

Copy link
Copy Markdown
Author

v2:

  • replace 'check' with 'identical'
  • fix test
  • fix doc
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MarcFinetRtone
merge: support 'identical' on overlapped data
5e2b05b 
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
@MarcFinetRtone
MarcFinetRtone requested a review from The-42 July 1, 2021 10:45
@MarcFinetRtone
MarcFinetRtone force-pushed the new-check-for-overlapping-data branch from 855fe8b to 5e2b05b Compare September 15, 2021 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@The-42 The-42 Awaiting requested review from The-42

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@The-42 The-42

Labels

enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@MarcFinetRtone @The-42
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.