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

Add the ability to hide CommandOptions #631

Closed
nils-a opened this issue Nov 22, 2021 · 3 comments · Fixed by #642
Closed

Add the ability to hide CommandOptions #631

nils-a opened this issue Nov 22, 2021 · 3 comments · Fixed by #642
Assignees
Labels

Comments

@nils-a
Copy link
Contributor

nils-a commented Nov 22, 2021

Is your feature request related to a problem? Please describe.
Coming from #629, it would be good to have the ability to hide CommandOptions.

Describe the solution you'd like
Something like

public class MySettings : CommandSettings
{
    [CommandOption("--hidden-opt", IsHidden = true)]
    public bool HiddenOpt { get; init; }
}

Additional context
See #629

@nils-a nils-a added the feature label Nov 22, 2021
@twaalewijn
Copy link
Contributor

@nils-a I basically have this working now.

I do have some minor things I would like feedback on before submitting the PR.

  1. I did not know about the hidden commands such as cli explain and cli xmldoc until I looked at the code. I have assumed the following behavior regarding this feature for those commands:
  • xmldoc seems to not output hidden commands so I have excluded the hidden options from the output of this command as well.
  • explain has a --hidden switch to also output hidden commands, I derived whether to output hidden options from that switch as well.

Did I miss anything else regarding those commands and is the behavior I summarized above correct?
Also I did not see any tests for explain, is there are reason for that and should I skip that or did they just not get made yet?

  1. I was going to just use init; on the IsHidden property inside CommandOption but, as I learned this morning, you cannot use init; for target frameworks that came before net5.0 unless you add some internal dummy type to trick the compiler into compiling anyway.
    I did not really like that as a fix so instead of using init; could you please specify which of the following you would prefer.

Just use a public setter since it is only an optional property inside of an attribute anyway:

public bool IsHidden { get; set; }

Or make it part of the constructor instead with a default value:

public bool IsHidden { get; }

public CommandOptionAttribute(string template, bool isHidden = false)
{
    IsHidden = isHidden;
}

// Usage
[CommandOption("--foo", true)]
[CommandOption("--foo", isHidden: true)]

Personally I would just go with the public setter since, for attributes, I would only expect required parameters to be on the constructor itself but maybe you would think differently about that to keep the property itself read only like the rest of them.

On a side note:

The CONTRIBUTING document specifies that you should not do things like reformat files which is understandable.

However I almost accidentally changed the encoding of a bunch of files just by opening them in Visual Studio and I also did not initially notice that all the files are using just LF for line endings instead of CRLF.

I would recommend adding stuff like that to the .editorconfig so the editor can catch these kinds of mistakes.
E.g.:

[*.cs]
end_of_line = lf
# This is is what VS2022 itself uses when you create a new '.NET' editorconfig file
charset = utf-8-bom

@nils-a
Copy link
Contributor Author

nils-a commented Nov 25, 2021

@twaalewijn

Did I miss anything else regarding those commands and is the behavior I summarized above correct?

Sounds good to me. If you missed something, I did, too. 😄

Personally I would just go with the public setter [...]

Yes, so would I.

However I almost accidentally changed the encoding of a bunch of files [...]

That's kind of strange. Are you using a non-default setting for core.autocrlf in git?
git ls-files --eol **/*.cs shows all files are checked out crlf on my system. Then again, this is what I would expect on a (windows) system with core.autocrlf set to true.

So, setting end_of_line to lf in .editorconfig would probably break every windows system which has core.autocrlf set to true. We could think about adding an .gitattributes file to set the line endings on checkout explicitly, but I'm not sure if that's really needed. (IMHO, Visual Studios' tendency to change line endings is the real culprit here.)

twaalewijn added a commit to twaalewijn/spectre.console that referenced this issue Nov 28, 2021
CommandOptions now has an IsHidden property that, when set to true, will cause the option to be hidden from the following cases:

- Help text using `-h|--help`
- Xml representations generated with the `cli xml` command
- Diagnostics displayed with the `cli explain` command

Hidden options can still be outputted with `cli explain` using the `--hidden` option that is also used to display hidden commands.

Fixes spectreconsole#631
@twaalewijn
Copy link
Contributor

My apologies, the whole .editorconfig/.gitattributes thing was indeed caused due to the way git was installed on the machine I was using.
I guess I never noticed it until now because all the other repositories I had cloned on that machine included a .gitattributes file with * text=auto.

I have created the PR for this feature now.
Please let me know if you need me to change or add anything to it.

nils-a pushed a commit that referenced this issue Nov 29, 2021
CommandOptions now has an IsHidden property that, when set to true, will cause the option to be hidden from the following cases:

- Help text using `-h|--help`
- Xml representations generated with the `cli xml` command
- Diagnostics displayed with the `cli explain` command

Hidden options can still be outputted with `cli explain` using the `--hidden` option that is also used to display hidden commands.

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

Successfully merging a pull request may close this issue.

2 participants