Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow strip and keep to be used together #580

Merged
merged 1 commit into from
Nov 18, 2023

Conversation

andrews05
Copy link
Collaborator

This is a minor change that allows using both --strip and --keep at the same time.

E.g. --strip safe --keep eXIf will strip chunks while preserving both the ones that aren't "safe" to remove and eXIf. Essentially it's a convenience to allow extending the default list used by --strip safe.

Specifying chunk names for both options is not permitted, e.g. --strip eXIf --keep eXIf will error.

Use of --strip all with --keep is redundant, but is permitted.

Copy link
Collaborator

@AlexTMjugador AlexTMjugador left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@AlexTMjugador AlexTMjugador merged commit 97af04a into shssoichiro:master Nov 18, 2023
@ace-dent
Copy link

@andrews05 - please can you update Docs and builtin help? 😇
It's not clear to me the order of priority /precedence. Can I do --keep aCEd --strip all to remove everything but my select chunk?

@andrews05
Copy link
Collaborator Author

andrews05 commented Nov 18, 2023

Heh, yeah I didn't think about docs until after I posted the PR. I'll make some changes...

Argument order is irrelevant, your example will keep the selected chunk. Perhaps we could prohibit keep with strip-all to reduce any confusion there?

@ace-dent
Copy link

Perhaps we could prohibit keep with strip-all to reduce any confusion there?

I think being able to combine both is useful. Perhaps stating that 'keep overrides strip' in the docs is sufficient?

@andrews05
Copy link
Collaborator Author

Hm, what makes you say it could be useful?
Normally invalid combinations would be prohibited. In the case of --strip all --keep it’s not so much invalid as redundant, but it would still be preferable to prohibit it rather than having to explain in the docs that keep will override if you do that.

@ace-dent
Copy link

ace-dent commented Nov 19, 2023

Perhaps I misunderstood...

--strip all --keep # (1) Useless!
--strip all --keep aCEd # (2) Has some use!?...

Not sure how complicated it is to distinguish between the 2 cases?...

... preferable to prohibit it rather than having to explain in the docs that keep will override [it] ...

Yes, ideally prohibit (1). But Docs should state keep overrides strip, as per (2).

@andrews05
Copy link
Collaborator Author

Sorry, I should have put a value in there - it is required to have one, so (1) is indeed already prohibited 😆

But I'm still unclear why you're suggesting --strip all --keep aCEd has some use. keep is overriding, so --strip all is necessarily redundant and you should just use --keep aCEd.

@ace-dent
Copy link

I should read the manual .... 🤦‍♂️ !!
Please ignore my comments, I was thinking of another program...

Yes: a combination of --strip all and keep should be prevented.
Sorry again 🙃

@andrews05
Copy link
Collaborator Author

Ha, all good 🙂
Not sure what keep would do if it didn't strip everything else? 🤔

Anyway, I'll make the changes...

andrews05 added a commit to andrews05/oxipng that referenced this pull request Nov 30, 2023
Pr0methean pushed a commit to Pr0methean/oxipng that referenced this pull request Dec 1, 2023
This is a minor change that allows using both `--strip` and `--keep` at
the same time.

E.g. `--strip safe --keep eXIf` will strip chunks while preserving both
the ones that aren't "safe" to remove *and* eXIf. Essentially it's a
convenience to allow extending the default list used by `--strip safe`.

Specifying chunk names for both options is not permitted, e.g. `--strip
eXIf --keep eXIf` will error.

Use of `--strip all` with `--keep` is redundant, but is permitted.
andrews05 added a commit to andrews05/oxipng that referenced this pull request Dec 10, 2023
AlexTMjugador pushed a commit that referenced this pull request Dec 13, 2023
This PR reverts #580 and introduces `--keep display` instead.

See discussion in #581.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants