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

Consistent behavior with missing or incorrect arguments #1179

Merged
merged 1 commit into from
Nov 2, 2023

Conversation

Rangi42
Copy link
Contributor

@Rangi42 Rangi42 commented Oct 25, 2023

Fixes #1131
Fixes #1132

  • Running rgbasm, rgblink, rgbgfx, or rgbfix with an incorrect argument (e.g. --foobar) will give an "unrecognized option" error, show the usage information, and exit.
  • Running rgbasm, rgblink, rgbgfx, or rgbfix without any arguments will give a fatal error, show the usage information, and exit.

Previously, running rgbfix without any arguments would default to processing - (stdin). Now you need to explicitly pass -.

@Rangi42 Rangi42 added rgbasm This affects RGBASM rgblink This affects RGBLINK rgbfix This affects RGBFIX rgbgfx This affects RGBGFX labels Oct 25, 2023
@Rangi42 Rangi42 requested a review from ISSOtm October 25, 2023 23:24
@ISSOtm
Copy link
Member

ISSOtm commented Oct 26, 2023

What good is rgbfix useful for without any arguments?

It may be worth aligning its behaviour with the other three, and mentioning "use rgbfix - if you want to simply pipe the input" or something.

@Rangi42
Copy link
Contributor Author

Rangi42 commented Oct 26, 2023

What good is rgbfix useful for without any arguments?

It may be worth aligning its behaviour with the other three, and mentioning "use rgbfix - if you want to simply pipe the input" or something.

I didn't want to accidentally break someone's workflow who's using < foo.asm rgbasm -o - - | rgblink -o - - | rgbfix > foo.gb instead of | rgbfix -. But yeah, if it's okay to break that negligible possibility, I'd rather have them be consistent.

@ISSOtm
Copy link
Member

ISSOtm commented Oct 26, 2023

Well, it's unlikely enough, we've had several reports of people being confused by RGBFIX's behaviour (appearing to hang), and we're not WG14, so I think it's worth breaking.

We'll have to add a note in the changelog to highlight the breakage as intentional for future reference, I guess.

@pinobatch
Copy link
Member

pinobatch commented Oct 26, 2023

A couple years ago, I wrote a compression tool in C to avoid a Python dependency in a replacement boot ROM. I checked whether isatty(fileno(stdout)) before trying to compress to standard output and whether isatty(fileno(stdin)) before trying to decompress from standard input. The user would get an error like this:

pb8: cannot compress to terminal; try redirecting stdout or pb8 --help

I had to do a bit of #ifdef at the top to make it work on both POSIX and Windows systems.

@aaaaaa123456789
Copy link
Member

Just break this; the fix is easy. But please do mention it, so people know how to fix this.

@Rangi42
Copy link
Contributor Author

Rangi42 commented Oct 27, 2023

Done: rgbfix is now consistent with the rest, and man pages explicitly mention -.

@Rangi42
Copy link
Contributor Author

Rangi42 commented Oct 28, 2023

I didn't want to accidentally break someone's workflow ...

Well, it's unlikely enough, we've had several reports of people being confused by RGBFIX's behaviour (appearing to hang), and we're not WG14, so I think it's worth breaking.

Just break this; the fix is easy. But please do mention it, so people know how to fix this.

Of course, the rgbfix tests themselves depended on this, and now they're all breaking. :P (Edit: should be fixed now.)

@Rangi42 Rangi42 force-pushed the command-help branch 2 times, most recently from d861491 to bb2dbbd Compare October 28, 2023 03:07
@Rangi42 Rangi42 added this to the v0.6.2 milestone Oct 28, 2023
@Rangi42 Rangi42 requested a review from avivace October 31, 2023 18:31
Copy link
Member

@ISSOtm ISSOtm left a comment

Choose a reason for hiding this comment

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

What's the rationale for making all of the "print usage" functions "noreturn"? It feels like it's obscuring the control flow IMO.

@Rangi42
Copy link
Contributor Author

Rangi42 commented Nov 1, 2023

What's the rationale for making all of the "print usage" functions "noreturn"? It feels like it's obscuring the control flow IMO.

Some of them already called exit() inside, and the ones that didn't were always immediately followed by exit() anyway. Would it help to rename them all exitUsage?

@aaaaaa123456789
Copy link
Member

I'd say this is pretty standard, because if you're printing usage information, you're not doing anything else. I've never seen a program print usage information and not exit right after.
Exiting from inside those functions is quite common, too.

@ISSOtm
Copy link
Member

ISSOtm commented Nov 1, 2023

It would make sense to print an extra line afterwards, for example. I'd rather have the flow control be explicit in the caller.

@Rangi42
Copy link
Contributor Author

Rangi42 commented Nov 2, 2023

It would make sense to print an extra line afterwards, for example. I'd rather have the flow control be explicit in the caller.

Okay.

ISSOtm
ISSOtm previously requested changes Nov 2, 2023
Copy link
Member

@ISSOtm ISSOtm left a comment

Choose a reason for hiding this comment

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

Great! The UX should improve that much overall. Thank you!

src/fix/main.c Outdated Show resolved Hide resolved
@ISSOtm ISSOtm merged commit 0d72ba8 into gbdev:master Nov 2, 2023
24 checks passed
@Rangi42 Rangi42 deleted the command-help branch November 2, 2023 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rgbasm This affects RGBASM rgbfix This affects RGBFIX rgbgfx This affects RGBGFX rgblink This affects RGBLINK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rgbfix appears to "hang" if no rom file is specified show help if rgbgfx is invoked without params
4 participants