feat: Update type parsing to use new diagnostics #605
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/diagnostics #605 +/- ##
====================================================
+ Coverage 91.75% 92.49% +0.74%
====================================================
Files 61 66 +5
Lines 6633 7250 +617
====================================================
+ Hits 6086 6706 +620
+ Misses 547 544 -3 ☔ View full report in Codecov by Sentry. |
| annotate_location(expr_ast, source, info.filename, 0) | ||
| annotate_location(expr_ast, source, info.filename, 1) |
There was a problem hiding this comment.
Drive-by: Our new spans start counting lines at 1
There was a problem hiding this comment.
As per documentation on Loc, indeed
There was a problem hiding this comment.
But columns starting at 0? :(
There was a problem hiding this comment.
Columns also start at 1, but that's the same in Python AST afaik, so it works out without having to modify it
guppylang/tys/errors.py
Outdated
| def rendered_title(self) -> str: | ||
| if self.expected > self.actual: | ||
| return "Missing type arguments" | ||
| elif 0 == self.expected < self.actual: |
There was a problem hiding this comment.
nit: this case is a bit confusing - I mean, the assumption is that expected>=0 and actual>=0 and expected!=actual in which case the < self.actual here is redundant. I'd just be consistent with rendered_span_label i.e. check self.expected == 0 first and then the other two.
There was a problem hiding this comment.
Alternatively the special case of expected == 0 might be neater as a separate error
| @@ -1,6 +1,8 @@ | |||
| Guppy compilation failed. Error in file $FILE:11 | |||
| Error: Invalid annotation (at $FILE:11:16) | |||
There was a problem hiding this comment.
driveby: at $FILE? Not at nonlinear.py or even at <stdin> ?
There was a problem hiding this comment.
This is only a placeholder in the golden value. The testing framework replaces it with the actual path (which will be different on every machine that cloned the repo)
| var_def: "ParamDef" | ||
|
|
||
| @dataclass(frozen=True) | ||
| class Explain(Note): |
There was a problem hiding this comment.
You might consider giving FreeTypeVarError a __post_init__ that does the self.add_sub_diagnostic(Explain(None)), as it looks easy for the caller (analyser) to forget the help message, and I'm not sure I see the Caller ever providing any parameter other than None (e.g. another span??)
There was a problem hiding this comment.
Good idea, there are a lot more places where we could do this 👍
See #620
| annotate_location(expr_ast, source, info.filename, 0) | ||
| annotate_location(expr_ast, source, info.filename, 1) |
There was a problem hiding this comment.
As per documentation on Loc, indeed
| annotate_location(expr_ast, source, info.filename, 0) | ||
| annotate_location(expr_ast, source, info.filename, 1) |
There was a problem hiding this comment.
But columns starting at 0? :(
No description provided.