API for registering errors, warnings and informational messages #160
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Mingun
commented
May 28, 2021
| const visitor = require("../visitor"); | ||
|
|
||
| // Removes proxy rules -- that is, rules that only delegate to other rule. | ||
| function removeProxyRules(ast, options) { | ||
| function removeProxyRules(ast, options, session) { |
There was a problem hiding this comment.
Actually, session seems to be more frequently used parameter, but adding it as the second would be a breaking change.
Comment on lines
+34
to
+58
| // In order to preserve backward compatibility we cannot change `GrammarError` | ||
| // constructor, nor throw another class of error: | ||
| // - if we change `GrammarError` constructor, this will break plugins that | ||
| // throws `GrammarError` | ||
| // - if we throw another Error class, this will break parser clients that | ||
| // catches GrammarError | ||
| // | ||
| // So we use a compromise: we throw an `GrammarError` with all found problems | ||
| // in the `problems` property, but the thrown error itself is the first | ||
| // registered error. | ||
| // | ||
| // Thus when the old client catches the error it can find all properties on | ||
| // the Grammar error that it want. On the other hand the new client can | ||
| // inspect the `problems` property to get all problems. | ||
| if (this._firstError === null) { | ||
| this._firstError = new GrammarError(...args); | ||
| this._firstError.stage = this.stage; | ||
| this._firstError.problems = this.problems; | ||
| } |
hildjj
requested changes
Oct 20, 2021
| // All problems if this error is thrown by the plugin and not at stage | ||
| // checking phase | ||
| this.stage = null; | ||
| this.problems = [["error", message, location, diagnostics]]; | ||
| } |
There was a problem hiding this comment.
I think format() should change as well, to do something with the problems.
There was a problem hiding this comment.
Changed. Now all problems from problems property printed each after aother, separated by the blank line:
error: problem 1
warning: problem 2
...
info: problem N
…and information messages
…tests Add new test: without main location and source
(review with "ignore whitespace changes" on)
Mingun
changed the title
|
I messed up and pushed a proposed change set to your branch without your permission. Apologies! Let me know how you want to move forward, please. |
|
All's fine. I leaved checkbox Allow edits and access to secrets by maintainers checked in the PR that is why you were able to do that. I approve your changes. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Phew. Finally all necessary PRs are merged and I can provide this.
That is implementation of a Session API that I've mentioned early (well, couple of days turned into couple of weeks... but it worth it). Although I feared that a second commit could have to break backward compatibility, with the help of a small trick it was possible to avoid this. As a result, it turned out a little dirty, but without breaking changes.
The new API allows passes to register warnings, informational messages and errors and we can collect multiply errors in one compilation attempt -- as much as possible.
I've tried to explain everything in the README.md, so I will not repeat myself here. Just see the diff of the README.md.
Although it would be great to have this PR in the same version where we have improved the errors, it is possible that the changes here are too fundamental, so I don't rush you.