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

Enables output of errorId #4097

Merged
merged 10 commits into from
May 29, 2023
Merged

Enables output of errorId #4097

merged 10 commits into from
May 29, 2023

Conversation

davidcok
Copy link
Collaborator

Adds --verbose to parsing and enabling it to emit the errorId and explanation

By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.

@@ -218,7 +218,8 @@ public class ConsoleErrorReporter : BatchErrorReporter {
errorLine += $" {msg} {tok.TokenToString(Options)}";
}

if (Options.Verbose && false) { // Need to control tests better before we enable this
if (Options.Verbose) { // Need to control tests better before we enable this
errorLine += "\nID: " + errorId;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a bit confusing to have just "ID:" on a separate line - less obvious that's it's an error id about the previous line. I think this would be clearer:

formatting1-3.dfy(1,0): Warning: File contains no code (ID: ...)

This would probably be even better, to put the id next to "Warning" or "Error", but may require more plumbing:

formatting1-3.dfy(1,0): Warning (ID: ...): File contains no code 

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll put it after the message, so the important part of the message is upfront.
It is still only included when --verbose is on, so it won't affect existsing tests, but will take a bit more sedding to tease it out of the message.

@@ -23,6 +23,7 @@ module A {
}
}
formatting1-3.dfy(1,0): Warning: File contains no code
ID:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say we shouldn't print this at all if there is no error code yet.

@@ -1,5 +1,6 @@
# Reading from symbols.transcript
bank.dfy(4,25): Warning: constructors no longer need 'this' to be listed in modifies clauses
ID: none
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is "none" coming from?

Copy link
Member

@robin-aws robin-aws left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving knowing that other pending PRs will add tests shortly (since there are now no changes to existing tests, which is good!)

@davidcok davidcok enabled auto-merge (squash) May 29, 2023 19:35
@davidcok davidcok merged commit f727afd into dafny-lang:master May 29, 2023
@davidcok davidcok deleted the cok-print-id branch May 29, 2023 21:59
atomb pushed a commit that referenced this pull request Jun 7, 2023
This PR removes ugly code that is no longer necessary thanks to #4044
and #4097

By submitting this pull request, I confirm that my contribution
is made under the terms of the MIT license.

Co-authored-by: Aleksandr Fedchin <fedchina@amazon.com>
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 this pull request may close these issues.

2 participants