Add to match this opening <thing> notes to the diagnostics
#862
Conversation
ahoppen
force-pushed
the
ahoppen/notes
branch
from
703bc67 to
753ceca
Compare
|
@swift-ci Please test |
ahoppen
force-pushed
the
ahoppen/notes
branch
from
753ceca to
9b54586
Compare
|
@swift-ci Please test |
| if let notes = spec.notes { | ||
| if diag.notes.count != notes.count { | ||
| XCTFail(""" | ||
| Expected \(notes.count) notes but received \(diag.notes.count): |
There was a problem hiding this comment.
Could print the expected notes as well?
There was a problem hiding this comment.
I explicitly didn’t print the expected notes because the test failure will point to where the DiagnosticSpec is declared and the expected notes will be shown there anyway. So I think there is very little value in printing the expected notes. This matches what we are doing for diagnostics as well.
There was a problem hiding this comment.
I had a comment there too :P. I generally like having everything in the error, makes it easier to attach them to issues and figure out the problem at a glance (rather than going to the test, comparing the output to what's in the test, etc). I don't feel particularly strongly about this though.
There was a problem hiding this comment.
I disagree but fair enough. If you want to add it, I won’t oppose you. 😜
| @@ -246,9 +328,10 @@ func AssertParse<Node: RawSyntaxNodeProtocol>( | |||
| Expected \(expectedDiagnostics.count) diagnostics but received \(diags.count): | |||
There was a problem hiding this comment.
Same comment as for notes, would be nice to print all expected as well.
| public let position: AbsolutePosition | ||
|
|
||
| /// A description of what this note is pointing at. | ||
| public let noteMessage: NoteMessage |
There was a problem hiding this comment.
I'm still not sure having an ID for note/diagnostic is really worth it. But we can keep it for now 🤷
There was a problem hiding this comment.
I still think it’s very little overhead at this point and once a use case comes up we will be glad that we have so we don’t need to do string matching for the error message.
For diagnostics like
expected '}' to end actorissue a correspondingto match this opening '{'This resolves 38 TODOs, down to 1025.