-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: Add span and diagnostics base classes #548
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/diagnostics #548 +/- ##
====================================================
- Coverage 91.09% 89.90% -1.20%
====================================================
Files 59 61 +2
Lines 6264 6347 +83
====================================================
Hits 5706 5706
- Misses 558 641 +83 ☔ View full report in Codecov by Sentry. |
assert file is not None | ||
assert line_offset is not None | ||
# line_offset starts at 1, so we have to do subtract 1 | ||
start = Loc(file, x.lineno + line_offset - 1, x.col_offset) |
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.
Loc
is meant to be 1-indexed, so it seems good that line_offset starts at 1?
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.
But x.lineno
also starts at 1
Both of these come from the python ast
module 😅
span_label: ClassVar[str | None] = None | ||
|
||
#: Message that is printed if no span is provided. | ||
message: ClassVar[str | None] = None |
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.
Why have this be a ClassVar and not just a field?
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.
Otherwise Python will complain that non-default fields follow default ones when defining a subclass. E.g.
@dataclass(frozen=True)
class TypeMismatchError(Error):
expected: Type
actual: Type
title: str = "Type mismatch"
span_label: str = "Expected `{expected}`, got `{actual}`"
This is because the field order is dermined by the base classes, so the title
and span_label
constructor arguments would actually come before the expected
and actual
args. We would need to do
@dataclass(frozen=True)
class TypeMismatchError(Error):
expected: Type
actual: Type
title: str = field(default="Type mismatch", init=False)
span_label: str = field(defaut="Expected `{expected}`, got `{actual}`", init=False)
which would be quite cumbersome and looks noisy. Making them ClassVar
s tells the @dataclass
decorator that these fields are static and shouldn't be initialised by the constructor.
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.
Okay, I think the disconnect is that you're intending for these classes to be inherited from rather than used directly, so it makes sense for a the TypeMismatchError
subclass to have a class variable.
Closes #536.