Skip to content

Change long form of save-image --ignore-app-descriptor - #974

Merged
SergioGasquez merged 5 commits into
esp-rs:mainfrom
DaneSlattery:dms_ignore_app
Dec 9, 2025
Merged

Change long form of save-image --ignore-app-descriptor#974
SergioGasquez merged 5 commits into
esp-rs:mainfrom
DaneSlattery:dms_ignore_app

Conversation

@DaneSlattery

Copy link
Copy Markdown
Contributor

Solves #973

@SergioGasquez SergioGasquez left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good, and it's something we definitely want to do, but this would be considered a breaking change that requires a major bump. We could support both ignore_app_descriptor and ignore-app-descriptor or just wait and merge this when working towards v5.

@SergioGasquez SergioGasquez added the breaking changes This issue/PR introduces major breaking changes label Nov 26, 2025

@SergioGasquez SergioGasquez left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you modify this PR to support both ignore_app_descriptor and ignore-app-descriptor while adding a TODO that the underscore version should be removed for v5??

@DaneSlattery

Copy link
Copy Markdown
Contributor Author

I've used visible_alias in the macro, which shows up like this in the help. It can also be a hidden alias if you prefer, to discourage future use

      --ignore-app-descriptor
          Skip checking whether the app descriptor is present in the image
          
          [aliases: --ignore_app_descriptor]

@SergioGasquez SergioGasquez left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! A visible_alias should be fine, thanks!

@SergioGasquez
SergioGasquez added this pull request to the merge queue Dec 9, 2025
Merged via the queue into esp-rs:main with commit ba2f16d Dec 9, 2025
32 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking changes This issue/PR introduces major breaking changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants