[WIP] Adding extra detail to error messages #1454
Conversation
|
I think this is at the point where it could use some review. |
There was a problem hiding this comment.
this is good idea to replace all the assertions with duplicated logic and have better error message.
Should it be checkConsistency or even assertConsistency (I assume it raises an exception if the equality is not observed).
There was a problem hiding this comment.
by default it does not, if run with Check.QuickThrowOnFailure that method will throw the exception when the Property returns a failing case.
|
I like those changes to get more user-friendly and useful error message in the core library and I like the pass of slight refactoring to reduce duplication / better encapsulation of the logic building those messages. 👍 |
|
@dotnet-bot test this please |
|
@KevinRansom I tried to set this up to make it easier for localization, but I'm not sure how to take this forward. The only places where any descriptive text is used apart from the names of the function's arguments are in the DetailedExceptions module and the axis & bound names for Array2D Is there anything else I need to do to get this PR merge-ready? |
|
|
||
| /// helper function that creates labeled FsCheck properties for equality comparisons | ||
| let consistency name sqs ls arr = | ||
| (sqs = arr) |@ (sprintf "Seq.%s = %A, Array.%s = %A" name sqs name arr) .&. |
There was a problem hiding this comment.
can you put ticks around the %A I.e. '%A' it allows us to see that a value is missing in the event of bugs.
|
@cloudRoutine localization is not needed for fieldnames, or argument names. |
|
|
||
| namespace Microsoft.FSharp.Core | ||
|
|
||
| module internal DetailedExceptions = |
There was a problem hiding this comment.
Perhaps AutoOpen to eliminate all of the opens? It seems like these are generally applicable.
|
The changes look good, I left a few comments to consider. Both @forki and @dsyme complain whenever I do these opportunistic cleanups within a PR that does a specific change. Note: I personally don't mind it honest. When the opportunistic cleanup is extensive like it is in this PR they both submit them as separate cleanup PRs. Don't change this one ... but next time if you see a cleanup harvest just submit it as a separate cleanup only PR. Thanks for taking care of this Kevin |
|
I'll keep the cleanup in mind for future work, thanks for the review |
|
btw I made all the changes you suggested |
|
@cloudRoutine Kevin |
4 participants
Implements more detailed error messages using formatted strings that contain the current error message and specifics about why the exception was thrown.
Described in detail - #1438
kevinr -- Link with whitespace changes ignored: https://github.com/Microsoft/visualfsharp/pull/1454/files?w=1