Issue14725 getopt better exceptions #3491
Conversation
|
LGTM |
| @@ -614,6 +615,24 @@ private void getoptImpl(T...)(ref string[] args, ref configuration cfg, | |||
| } | |||
| } | |||
|
|
|||
| private void handleConversation(R)(string src, R* receiver, | |||
There was a problem hiding this comment.
What do you mean by "conversation"? Did you mean conversion?
| @@ -614,6 +615,24 @@ private void getoptImpl(T...)(ref string[] args, ref configuration cfg, | |||
| } | |||
| } | |||
|
|
|||
| private void handleConversion(R)(string src, R* receiver, | |||
| immutable size_t idx, string option, string file = __FILE__, | |||
| long line = __LINE__) | |||
| @@ -614,6 +615,24 @@ private void getoptImpl(T...)(ref string[] args, ref configuration cfg, | |||
| } | |||
| } | |||
|
|
|||
| private void handleConversion(R)(string src, R* receiver, | |||
There was a problem hiding this comment.
here I would keep the order of parameters like of handleOption.
void handleConversation(R)(string option, R receiver, string value, size_t index,
string file = __FILE__, size_t line = __LINE__`)
burner
force-pushed
the
issue14725
branch
2 times, most recently
from
86be56a
to
e3858bd
Compare
| else | ||
| { | ||
| handleConversion(option, val, receiver, i); | ||
| } |
There was a problem hiding this comment.
While I agree that the original code is hard to read, expanding it into 3-line braced blocks seems excessive. What about just leaving out those braces?
There was a problem hiding this comment.
I.e., leave the if-block and else-block as separate lines, but just indent them without using braces.
|
LGTM, @DmitryOlshansky I'll let you pull, if you still think it looks good after all the changes. |
|
Let's wait a bit on this. I have a proposal brewing that may solve this and many exception-hierarchy issues. |
|
could you post a reference? |
|
I'm on holiday for a good 2 weeks, I guess we can wait some more |
And I was about to post that I'm on a cellphone for at least a week. Let's get back to this in ~ 10 days. |
|
@DmitryOlshansky ping |
some simplifications first round of *receiver replacement only two places of "to!" left whitespace spell error dcarp fixes dcarp fixes 2 whitespace quickfur
|
@burner got busy with real life. Ping me back in a couple of days, I'll prepare the proposal |
|
ping |
|
well, the bugfix is still good. |
Simply put the current fix silently changes exception type. Now users catching ConvException would be surprized to see that they no longer catch the right condition. As for magic - yes I have it, have patience my friends. |
To start on this as I think I'll wait forever to write a DIP. What we should have here instead of this "fix" is throw an exception that is both ConversionException (semantics) and GetOptExcpetion (origin). I believe that I found a way to frutifully combine 2 exception hierarchies where one of them is going to be an interface hierarchy. Now my proposal wouldn't stop on just getting hierarchies right we should cover. Easy creation of exceptions : alias MyParseException = StandardException!(ParseException, my.module);
alias MyLogicException = StandardException!(LogicException, my.module);Ease of throwing and creating exceptions: // writef style formatting
throw exceptionf!MyParseException("This is lazily formatted with %d a number", 42);
// write style formatting
throw exception!MyParseException("This is lazily formatted with ", 42, " a number");Catching can be done by any of the two: semantic type as in ParseException or by origin (ideally) as try{ ... }
catch(ParseException ){ // may be a parse error, possibly in SQL statement
}
catch(OriginExcpetion!(arsd.mysql)) { // some exception from Mysql package as a whole
}I believe it's this blend of origin and semantic meaning is the way to go. It's novel I admit but it enables excellent idioms and seems to scale way beyond other hierarchies proposed. |
The missing piece is proof of concept and that may take a bit of time |
|
I remember you posted about this in the forum, but there wasn't very much response. Personally I think it's a great idea. |
|
@DmitryOlshansky ping? |
|
@DmitryOlshansky If this PR is fixing a genuine problem, then why not pull this instead of waiting for something that might still take a long time. I believe you when you say you have a better solution, but this solution exists now, why not just override this with your changes when they're ready? |
|
This just changes one exception for another - a breaking change that helps users that didn't expect ConvError but breakscode of others that expect it. Closing one ticket foranew one |
|
Yeah, changing the type of a thrown exception breaks code, so let's do it as few times as possible. I agree with @DmitryOlshansky; I think this patch is better applied after his work, when it will be backwards compatible. |
|
@DmitryOlshansky any news on you DIP |
Not yet, though I intend to process my D-language related backlog soonish. |
ping @DmitryOlshansky - ´I like your idea of having multiple exceptions. Any way we can help you? |
Well so far an attempt to convert Throwable to interface instead of a class exploded in my face at run-time. I need it done and it's the only real obstacle. I will publish the fork of druntime in case there is any interest |
|
If this is waiting on @DmitryOlshansky's work, then IMO this should be closed, as this will require a rewrite anyway. Right now this is just eating auto-tester time. |
|
agreeded |
https://issues.dlang.org/show_bug.cgi?id=14725