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

fix option parsing of --binary on non-Windows platforms #3131

Merged
merged 1 commit into from
May 29, 2024
Merged

fix option parsing of --binary on non-Windows platforms #3131

merged 1 commit into from
May 29, 2024

Conversation

calestyo
Copy link
Contributor

-b/--binary should be accepted on all platforms so that they can be used in code that is meant to be portable.

`-b`/`--binary` should be accepted on all platforms so that they can be used in
code that is meant to be portable.

Signed-off-by: Christoph Anton Mitterer <[email protected]>
@wader
Copy link
Member

wader commented May 29, 2024

Hey, thanks! look good to me. I wonder if we should add a regression test for this in shtest?

@calestyo
Copy link
Contributor Author

For this particular case I, personally, would rather it's not worth a test.

The test would only tell when the option parsing itself fails again, which I guess is unlikely, but even if, it would be something that's quickly noticed.

I'd rather like to see tests that check whether the different combinations of --binary and the --raw* options produce the same results per platform and with single/multiple values returned and with values which contain encoded LF and/or CRLF-newlines (see the point I've mentioned in #3132).

Copy link
Contributor

@itchyny itchyny left a comment

Choose a reason for hiding this comment

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

LGTM

@itchyny itchyny merged commit 5208a44 into jqlang:master May 29, 2024
28 checks passed
@calestyo calestyo deleted the fix-binary-option-on-non-windows branch May 30, 2024 00:19
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