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

feat(ParseResults): Alias *PostProcessResult* -> *GetResult* #211

Closed
bartelink opened this issue Dec 15, 2023 · 4 comments · Fixed by #230
Closed

feat(ParseResults): Alias *PostProcessResult* -> *GetResult* #211

bartelink opened this issue Dec 15, 2023 · 4 comments · Fixed by #230

Comments

@bartelink
Copy link
Member

Preamble:

I have used Argu for many years but PostProcessResult always scared me (and I had not seen it organically).

The naming put me off - it sounds like something some metaprogrammer genius would be doing in some esoteric scenario.

In recent times, I actually saw/looked at the tutorial for the first time, and realised it was actually quite useful (I used to do Option.map and think no more of it, but having exceptions be trapped and reported wrt that specific argument is very desirable)

The other issue is that short names matter


Now that there is a GetResult with a fun () -> <default>, it would seem some additional overloads would be useful:

  • GetResult<T>(Thing, fun x -> <map x to T>) : 'T (aka PostProcessResult)
  • TryGetResult<T>(Thing, fun x -> <map x to T>) : 'T option (aka TryPostProcessResult)
  • GetResult<T, U>(Thing, (fun () -> <default Thing : T>), fun x -> <map x to U>) : 'U
  • GetResult<T, U>(Thing, <default Thing: T>, fun x -> <map x to U>) : 'U (if I can only have one, I'd take the preceding one, but its way less parens)

The main questions for me are:

  • does anyone other than me see the point of these extensions? I have real code that would benefit from these a lot...
  • is the current name actually better anyway
  • is Argu just too old.mature to change something fundamental like this?
  • would the overloading cause mess/conflict in real code

I propose to:

  • add the overloads now, possibly as a -rc so it can be road-tested
  • In V7, Obsolete PostProcessResult/TryPostProcessResult (with recommendation to use GetResult/TryGetResult instead)
@nojaf
Copy link
Collaborator

nojaf commented Dec 18, 2023

Hi, this proposal seems reasonable.

I don't have the biggest issue with the current naming after reading the tutorial. But I don't think I've used this anywhere myself, so I'm probably not a good candidate to offer feedback here.

What is the benefit of passing the mapping to Argu instead of piping the result afterwards?

let x = args.GetResult(Thing) |> mapThing

I'm just trying to understand things better.

@dawedawe
Copy link
Member

Hey @bartelink , thanks for making me aware of PostProcessResult.
I might have a use case that could profit from it 😃

@bartelink
Copy link
Member Author

What is the benefit of passing the mapping to Argu instead of piping the result afterwards?

Key difference is that exceptions are trapped and mapped to ArguParseException (the existing Catch method enables that generically, and anything that takes a function traps it similarly) - and it's contextual, i.e. if defaulting for some nested item needs to say "we can't load this secret", you're not producing a top level help message only

The main question here is whether to focus on making the API intuitive for intellisense discovery should matter, as its never ideal to change names

The other thing in play is that these defaulting / fallback mechanisms is a lower level tactical mechanism - it's conceivable that #107 / #65 / #143 (if implemented) might dictate another mechanism and render this a point solution that distracts from a higher level approach

@bartelink
Copy link
Member Author

Am thinking I'm going to do a v6.2 PR fairly soon that adds the GetResult overloads that alias PostProcessResult per the OP here

And add a Factory method that does command line parsing only with more general program name derivation (see #171 (comment))

Then in V7 the PostProcess stuff gets obsoleted, and in V8 they get removed

If anyone else has things that are confusing and/or want to pave the way for future changes, it might be useful to do them as a batch in a 6.2 release cc @Numpsy

@bartelink bartelink changed the title PostProcessResult->GetResult feat(ParseResults): Alias PostProcessResult->GetResult Feb 19, 2024
@bartelink bartelink changed the title feat(ParseResults): Alias PostProcessResult->GetResult feat(ParseResults): Alias *PostProcessResult* -> *GetResult* Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants