search

dotnet / command-line-api

Command line parsing, invocation, and rendering of terminal output.
https://github.com/dotnet/command-line-api/wiki
MIT License
3.34k stars 375 forks source link

Make StringExtensions a standalone class as opposed to an extension class #2436

Closed PonchoPowers closed 4 weeks ago

PonchoPowers commented 4 weeks ago

I can't see any benefit in making this a string extension, why not just create a class and pass the args into a method? In all cases I can think of, being an extension for a string is misleading, as this is specific to args, not strings.

elgonzo commented 4 weeks ago

Nothing forces you to use an extension method only like an extension method. If you need or want, you can use an extension methods just like an ordinary static method.

If your concern is with Intellisense suggesting these methods, note that any of the StringExtensions classes found in System.CommandLine (particularly in the current development branch "main-powderhouse") are marked internal with the exception of DragonFruit. This should render your concern moot as far as these classes are concerned.

If you are talking about DragonFruit's StringExtensions class (which is public), there should be no need for your app to import the System.CommandLine.DragonFruit namespace (thus Intellisense shouldn't suggest methods of this class either.

PonchoPowers commented 4 weeks ago

My concern is that StringExtensions is an extension method that applies to all strings, but not all strings are arguments, nor is this a proper use case for extension methods either. The intention isn't clear from the name, the code isn't the cleanest and given the standard held over the .NET Framework, I'm sure others would have something to say about the class too. I know how extension methods work and I know the internal keyword is used, the point is however that a standalone class would be more appropriate. What is the use case for using extension methods in this case, other than just because you can?

PonchoPowers commented 4 weeks ago

To put it into perspective, Tokenize should be split out into a class, and the CanBeUnbundled and TryUnbundle methods moved into the new class, but instead of nesting these methods, they should be separate so that they can be unit tested, how it is written currently makes testing difficult and also diminishes the value of these methods, which are infinitely useful for dealing with command lines, the use case at the moment seems too narrow minded as to parsing when the library could also be useful for generating commands too.

elgonzo commented 4 weeks ago

[...] which are infinitely useful for dealing with command lines, the use case at the moment seems too narrow minded as to parsing when the library could also be useful for generating commands too.

I am afraid i don't follow you. What is useful about these methods in a way that you wouldn't be able to (relatively easily) achieve through one of the public methods provided by CliParser? "generating commands"... what exactly do you mean? Commands are generated by instantiating and setting up CliCommand instances.

To put it into perspective, Tokenize should be split out into a class, and the CanBeUnbundled and TryUnbundle methods moved into the new class, but instead of nesting these methods, they should be separate so that they can be unit tested,

I might agree about the current Tokenizer.Tokenize method implementation not being test-friendly (although i haven't looked at it closely, so i'd like to defer my judgement about it), but i fail to see how this is related to your OP that talked about some internal string-based extension methods preferably not being internal and not being extension methods. I guess this should be a topic on its own instead of lumping it with this thread here turning it into a meandering discussion about this and that...

PonchoPowers commented 4 weeks ago

Not sure if we are looking at the same code, so I've included a screenshot, as Tokenize is in the StringExtensions from what I can see, not sure where you are getting Tokenize.Tokenize from...

image

elgonzo commented 4 weeks ago

Not sure if we are looking at the same code

Yes, we are. The source file is named StringExtensions.cs. However, the Tokenize method is in the Tokenizer class: https://github.com/dotnet/command-line-api/blob/2946046fadd284ba7d2ed92e8bab68f5e5dac31f/src/System.CommandLine/Parsing/StringExtensions.cs#L32

PonchoPowers commented 4 weeks ago

I was looking at main, I will look at the powderhouse branch, but at the moment it is very much in a state of flux, last time I looked a lot of code had been commented out so I was making use of main.

PonchoPowers commented 4 weeks ago

I think this can be closed as what I've proposed has already been done in powderhouse.