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

CommandLineApplication code smell - has too many APIs #298

Closed
natemcmaster opened this issue Oct 30, 2019 · 6 comments
Closed

CommandLineApplication code smell - has too many APIs #298

natemcmaster opened this issue Oct 30, 2019 · 6 comments
Labels
breaking-change closed-wontfix This issue is closed because there are no plans to fix this. discussion needs-design

Comments

@natemcmaster
Copy link
Owner

natemcmaster commented Oct 30, 2019

CommandLineApplication has grown into a god class. It has 116 APIs on it. I'd like to consider ways to refactor CommandLineApplication to simplify it.

The entire API as of 2.4: https://gist.github.com/natemcmaster/b524f050c7124fcf92011025a75243d3

Ideas:

  • Make breaking change to CommandLineApplication and move APIs into separate classes based on logical grouping
  • Leave CommandLineApplication as-is, but implement a refactored API surface and shoe-horn CommandLineApplication into the new API. Update docs and samples to use the new API and consider deprecating CommandLineApplication in the long run

Logical grouping

  • parser configuration
    • AllowArgumentSeparator
    • ClusterOptions
    • MakeSuggestionsInErrorMessage
    • OptionsComparison
    • ResponseFileHandling
    • ThrowOnUnexpectedArgument
  • handling the parsing result
    • Parse()
    • OnParsingComplete()
    • RemainingArguments
    • ValueParsers
  • defining the structure of the commands and options/arguments
    • AddSubcommand()
    • Command()
    • Comamnds
    • Argument() and Argument()
    • Arguments
    • Option() and Option()
    • Options
    • GetOptions()
    • HelpOption()
    • OptionHelp
    • OptionVersion
    • VersionOption()
    • VersionOptionFromAssemblyAttributes()
    • Parent
    • Model
    • ModelFactory
    • Conventions
  • help text
    • ShowHelp()
    • ShowVersion()
    • ShowRootCommandFullNameAndVersion()
    • HelpTextGenerator
    • UsePagerForHelpText
    • LongVersionGetter
    • ShortVersionGetter
    • ShowInHelpText
    • ExtendedHelpText
    • FullName
    • Names
    • Name
    • AddName
    • Description
  • validation
    • GetValidationResult()
    • OnValidationError()
    • ValidationErrorHandler
    • Validators
  • app lifecycle and command execution context
    • IsShowingInformation
    • WorkingDirectory
    • Invoke
    • OnExecute() and OnExecuteAsync()
    • Execute() and ExecuteAsync()
    • Out
    • Error
@natemcmaster
Copy link
Owner Author

Ping! Anyone have thoughts on this one? Agree, disagree?

@Miista
Copy link

Miista commented Dec 8, 2019

I think it would be a good idea to break up CommandLineApplication.

With regards to the ideas you proposed I feel that leaving CommandLineApplication as-is and shoe-horning it into a refactored API surface would just push the problem into the future.
I would much rather we make a breaking change, introducing a clean API.
How it should be grouped could be discussed further.

@natemcmaster
Copy link
Owner Author

Maybe I'm overly cautious as a result of starting my career on the .NET team at Microsoft, but I have a preference for minimizing breaking changes -- or at the very least, doing it gracefully. What do you think about a two-step breaking change?

Step 1 - design the new API and release it in 3.0. Mark CommandLineApplication with [Obsolete("<INSERT REASON HERE>. See more details at <LINK>.")]. This gives users compiler warnings and instructions how how to upgrade.
Step 2 - delete CommandLineApplication in release 4.0.

Here's an example of one place I've used this approach:

/// This constructor is obsolete. The recommended replacement is <see cref="SubcommandAttribute(Type[])"/>.
/// </summary>
/// <param name="name">The name of the subcommand</param>
/// <param name="commandType">The type of the subcommand.</param>
[Obsolete("[Subcommand(string, Type)] is obsolete and will be removed in a future version. " +
"The recommended alternative is [Subcommand(Type)]. " +
"See https://github.com/natemcmaster/CommandLineUtils/issues/139 for details.")]

@Miista
Copy link

Miista commented Dec 10, 2019

That seems like a good approach. My worry was that you would keep the shoe-horned CommandLineApplication alongside the new API.

@stale
Copy link

stale bot commented Jul 21, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Please comment if you believe this should remain open, otherwise it will be closed in 14 days. Thank you for your contributions to this project.

@stale stale bot added the closed-stale This issue is closed because it went stale and there was no action planned. It can be reopened. label Jul 21, 2021
@github-actions github-actions bot removed the closed-stale This issue is closed because it went stale and there was no action planned. It can be reopened. label Nov 14, 2021
@natemcmaster
Copy link
Owner Author

Closing as stale. At this point, I have no intention in addressing this problem.

@natemcmaster natemcmaster added the closed-wontfix This issue is closed because there are no plans to fix this. label Dec 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change closed-wontfix This issue is closed because there are no plans to fix this. discussion needs-design
Projects
None yet
Development

No branches or pull requests

2 participants