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

Move Parse extension methods to the appropriate Symbol types #1901

Closed
Tracked by #1891
jonsequitur opened this issue Nov 2, 2022 · 6 comments
Closed
Tracked by #1891

Move Parse extension methods to the appropriate Symbol types #1901

jonsequitur opened this issue Nov 2, 2022 · 6 comments
Labels
Milestone

Comments

@jonsequitur
Copy link
Contributor

jonsequitur commented Nov 2, 2022

        // This method group looks fine, but should be an instance method on Argument, or a non-extension method somewhere else.
        // I recommend using different names for the two overloads.  That would allow you to use `params string[] args` for the latter.
        // Argument::Parse(string) and Argument::ParseArguments(params string[] args), maybe?
        public static ParseResult Parse(this Argument argument, string commandLine);
        public static ParseResult Parse(this Argument argument, string[] args);
    }
@KalleOlaviNiemitalo
Copy link

Call it ParseArgumentList so it is similar to ProcessStartInfo.ArgumentList.

@jonsequitur
Copy link
Contributor Author

One possible concern there is that "argument" means something more specific in the context of the parser.

@KalleOlaviNiemitalo
Copy link

params string[] args doesn't seem that useful here, anyway. Callers would usually have the args in a collection object already, except perhaps in tests where you can just do new[] { "…", … }.

@jonsequitur jonsequitur added this to the 2.0 GA milestone Nov 9, 2022
@jonsequitur
Copy link
Contributor Author

params string[] args doesn't seem that useful here, anyway. Callers would usually have the args in a collection object already, except perhaps in tests where you can just do new[] { "…", … }.

And we've been trying to discourage testing that way because it requires you to know how the command line input string will be split into an array of strings, which is not at all straightforward: #1758.

@KalleOlaviNiemitalo
Copy link

It's well understood for POSIX sh.

@adamsitnik
Copy link
Member

fixed in #1951

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

No branches or pull requests

3 participants