Conversation
benfdking
force-pushed
the
adding_range_to_linting_errors
branch
from
8042afa to
0109528
Compare
| class Position(PydanticModel): | ||
| """The position of a rule violation in a file, the position follows the LSP standard.""" |
There was a problem hiding this comment.
Why do these wrapper classes inherit from PydanticModel instead of making them data classes? I think the latter is more lightweight, so may be worth exploring if we don't care about serializing these objects.
There was a problem hiding this comment.
🤷 I think serializing could quickly become useful but as you say easier to add after.
| class Range(PydanticModel): | ||
| """The range of a rule violation in a file. The range follows the LSP standard.""" | ||
|
|
||
| start: Position | ||
| end: Position |
There was a problem hiding this comment.
Should we document that this represents a [start, end) range?
There was a problem hiding this comment.
To me this is really what I was hoping
The range follows the LSP standard.
^ I don't want to reinvent the wheel, I just want them to follow the LSP standard. So if you are confused, just go there.
| line: int | ||
| character: int |
There was a problem hiding this comment.
Does "character" mean column offset in the line? Why not use column or something similar so that the naming is consistent?
I also suggest we start documenting some details, like whether it's 1-based indexing and stuff, so reading the docstring makes it obvious to understand what's going on.
There was a problem hiding this comment.
idem, just want to specify LSP standard
| def _get_range(self, model: SqlModel) -> t.Optional[Range]: | ||
| """Get the range of the violation if available.""" | ||
| try: | ||
| if len(model.query.expressions) == 1 and isinstance(model.query.expressions[0], Star): |
There was a problem hiding this comment.
Should this instead call find(Star) on the model's query? Note that SQL like t.* won't be picked up by this check because the projection is a Column that contains a Star child in this.
There was a problem hiding this comment.
I think this can be improved, but I'm hesitant to do so right away because it adds complexity. For me, I'm just in the simplest case of returning a range, which I think we can expand on over time.
There's also a lot of other changes so rather do this later.
benfdking
force-pushed
the
adding_range_to_linting_errors
branch
2 times, most recently
from
747db1b to
a6ee7b2
Compare
benfdking
force-pushed
the
adding_range_to_linting_errors
branch
from
a6ee7b2 to
7009d81
Compare
benfdking
force-pushed
the
adding_range_to_linting_errors
branch
from
7009d81 to
bb2923f
Compare
- adds ability for linter to return range to specify where the error gets returned - maps that correctly over for the lsp - for the builtin selectstar rule, add the range
benfdking
force-pushed
the
adding_range_to_linting_errors
branch
from
bb2923f to
9b23b60
Compare
3 participants
gets returned