-
Notifications
You must be signed in to change notification settings - Fork 293
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
Removing main side effects from public interface (Parser type) #59
Comments
The downside to this approach is that the Options class must have a default constructor. This is probably a good idea anyway, but something to keep in mind. |
@nemec, I'm doing some experiment on anther branch, for now it seems promising. Here the branch (uncomplete), https://github.com/gsscoder/commandline/tree/issue59-sideeff Anyway the latest stable is tagged and well defined, https://github.com/gsscoder/commandline/tree/v1.9.71.2 Thanks for the comments! PS: I've explained in detail all these things for future reference / other users, I know you're more than familiar with what I quoted. |
Conflicts: CommandLine.sln.DotSettings.user README.md src/SharedAssemblyInfo.cs
Work is moved to new official beta https://github.com/gsscoder/commandline/tree/develop-1.9.8-beta branch. Some was done, other things must be done, but I think that this could be a nice starting point. The issue could be considered close (also if other msg maybe appended here). When some internal refactoring goes to end -> it will be merged to master. Contributors: #62. A nice day to everyone! :)) |
I want to add. The public API of T ParseArguments<T>(string[] args, Action onFail) where T : new()
T ParseArguments<T>(string[] args, Action<string, object> onVerbCommand, Action onFail) where T : new() A lot cleaner now... or not? |
@nemec, We can do as most IoC does, providing an overload like T ParseArguments<T>(Func<T> builder, string[] args, Action onFail) so that var parser = new Parser();
var options = parser.ParseArguments<AbstractOptions>(() => new ConcreteOptions(), args, () => Environment.Exit(-1));
// we are here, options can't be null in any case! :) What do you think about? It avoid side effects on options instance and let devs create it... |
That would work. Of course it's easy to introduce side effects, but that's the implementer's problem rather than yours ;)
|
@nemec, great! you've read my mind... This is exactly what I've thought about it. Talking of side side effects (or intended effects as printing help) I think they are not an absolute evil. The same F# I'm not try to change this library to a purely functional piece a software (inside/out), but a stated in the post I'd like to remove all not benign side effects from public surface of the API. Always toward usability and a not-verbose-syntax compromise. PS: is also true, if you look to the new branch, that at a lot is done internally / and other can and will be done. |
As you seen with latest release a lot was done to simplify the library.
Don't get me wrong, I'll not change basic concepts of the API; but I'd like to embrace good design from functional world for make our library strongest.
When you declare the options type:
options
is in state X (initialized) and afteroptions
is in state Y (with parsed values)So we can say that
ParseArguments
caused side effects. Another one, is the result (also influenced by the same function... opssss method!).Incredibly the solution is quite easy:
The method with this new signature constructs the instance and returns it.
ParserStateAttribute
is used, the generic errors list will be available as always.But if parsing fails an appropriate delegate (
Action onFail
) is called.Other advantages: convergence of standard methods with strict methods (@nemec suggested feature).
Functional paradigms proves their effectiveness from '70 (and before), please dig into the argument if not convinced. :))
As always, opinions (or flames) are welcome.
PS:
Some side effects for the moment will remain. E.g.: printing out the help. As said, I'm moving to a removing the most evident of these.
The text was updated successfully, but these errors were encountered: