-
Notifications
You must be signed in to change notification settings - Fork 833
Better anonymous record reporting #8094
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
Conversation
cartermp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This is a great change to the error messages. Just some style nits.
Requesting changes since there are some small test updates that need to be made in the baseline files. You can see which ones here: https://dev.azure.com/dnceng/public/_build/results?buildId=472147&view=ms.vss-test-web.build-test-results-tab&runId=15028460&resultId=100248&paneView=debug
Since this is an error message update there's nothing special here, just copy/pasting the new expected error message into the baseline. It's kind of annoying.
This can be made less annoying, following the instructions there: |
|
@smoothdeveloper I couldn't actually figure out what to do with that script or how to run those BSL tests locally (they weren't even in the solution). How do I do it? |
|
@isaacabraham, have you been able to run tests under: https://github.com/dotnet/fsharp/blob/master/tests/fsharp/tests.fs ? after those ran, the .err files will contain the actual output, you can then adjust the script (it handles the folders explicitly, you need to put the folder names that you are interested in) and run it to copy the .err into .bsl files, review and commit/push. edit: running the script (with folder of the test added)
running the test again: |
cartermp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work, thanks!
In the original issue I proposed an alternative text, but it looks like you already made it far better than the original idea. I do have a few suggestions, though, feel free to use them, or not ;)
Error messages in F# typically don't use determiners, for consistency, I suggest to simplify it like this (which also removes a redundancy): "Anonymous record misses fields '%s' and has unexpected fields '%s'."
Same here, I'd suggest: "Anonymous record misses fields '%s'."
And here it would become: "Anonymous record has unexpected fields '%s'."
Perhaps something like: "Unexpected anonymous record. The expected anonymous record has fields: '%s'." |
|
@abelbraaksma I'll give it a bash and see how it "looks" locally. A few ideas I had in the past days looked good on paper or in my mind but when I tried them, they didn't print out as well. We can always iterate on this in the future - I don't believe there's a "no breaking changes" on error messages. |
I agree, even more so for compiler errors. |
While I said that myself, this turns out to be untrue, we do have errors that start with "this expression .... " etc. Right now I'm not so sure if there's any rule-of-thumb w.r.t. grammar/style/wording of errors. |
|
@isaacabraham is the error message itself how you'd like it? |
|
I'm personally satisfied with it, at least as a means to get feedback from others. It should be consistent enough that tooling could generate fixes on top of it as well. |
|
I'll merge this. The message is easy to adjust after the fact, so let's get it in! |
* Initial attempt at better anonymous record reporting. * Update error texts and fix tests. * Fix test, but for real this time. * Fix another random BSL file. * Next random file fix! * Line formatting.


Fixes #8091. This provides custom error messages for four cases of mismatched anonymous records:
Wording might need to be changed!