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

argparse: tell default renderer #4

Closed
wants to merge 2 commits into from

Conversation

@illwieckz
Copy link
Member

commented Jul 30, 2019

That's a very minor change, it makes built-in help displaying this:

Renderers:
  --daemon              Use renderer features of the Daemon engine. Makes the
                        shaders incompatible with XreaL and Quake3. (default:
                        False)
  --xreal               Use renderer features of the XreaL engine. Makes the
                        shaders incompatible with Quake3. (default: --xreal)
  --quake3              Use renderer features of the vanilla Quake3 engine
                        only. (default: False)

instead of this:

Renderers:
  --daemon              Use renderer features of the Daemon engine. Makes the
                        shaders incompatible with XreaL and Quake3. (default:
                        False)
  --xreal               Use renderer features of the XreaL engine. Makes the
                        shaders incompatible with Quake3. This is the default.
                        (default: False)
  --quake3              Use renderer features of the vanilla Quake3 engine
                        only. (default: False)

Notice the weird “This is the default. (default: False)” that was there before.

@Viech

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

I don't find this new help text more helpful than the other. The values --xreal and False don't quite compare, it's rather confusing. This is the default. seems more explicit to me.

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2019

The thing is that the (default: False) string was not present in previous versions of Python, and since it's added it now looks weird to read in the same sentence:

This is the default. (default: False)

The wording also implies that --xreal and False would be the same, which is weird… It's not a Sloth issue in any way, it's a Python/argparse one.

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2019

Wait… what if I set the default to True 😁

@Viech

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

Well, then passing another flag would need to revert it to False, no? Otherwise there should be a conflict since these options are exclusive. Maybe the new argparse also has a new way to show this properly. Otherwise I'd rather leave it as is, then it is at least consistently broken like other tools.

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2019

Oh yes, we can't set it to true because of the mutual exclusion… « consistently broken » well…

A better way to do it would have to do something like --engine xreal with xreal being the value, but doing that breaks the user interface. This is not a big issue since this tool is confidential, but I will not do it in this PR. I close this PR until some idea comes to my mind.

@illwieckz illwieckz closed this Aug 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.