Conversation
Codecov Report
@@ Coverage Diff @@
## master #78 +/- ##
==========================================
- Coverage 90.55% 87.34% -3.22%
==========================================
Files 31 32 +1
Lines 1440 1501 +61
Branches 227 241 +14
==========================================
+ Hits 1304 1311 +7
- Misses 118 172 +54
Partials 18 18
Continue to review full report at Codecov.
|
tkers
force-pushed
the
basic-linter
branch
2 times, most recently
from
e526a0b to
b5b67ec
Compare
tkers
force-pushed
the
basic-linter
branch
2 times, most recently
from
4c384de to
c50e213
Compare
davazp
left a comment
davazp
left a comment
There was a problem hiding this comment.
I find confusing a little bit what we consider a binding. Firstly it seemed it was only local variables + function arguments. But then module did extract definitions.
A couple of alternative ideas to make it more clear:
- Remove syntaxBindings (do it inside module directly).
- Define 2 kind of bindings, lexical-global.
| return expressionBindings(s); | ||
| } else { | ||
| switch (s.node.tag) { | ||
| case "definition": |
There was a problem hiding this comment.
Should definition also include a binding?
There was a problem hiding this comment.
These are global bindings, so handled by the moduleBindings. This function returns the bindings that only exist within the node.
| switch (s.node.tag) { | ||
| case "definition": | ||
| case "export": | ||
| case "type-alias": |
There was a problem hiding this comment.
I get the rule is unused-variables, but maybe it would make sense to extend it to unused types as well?
| } | ||
|
|
||
| function moduleBindings<I>(m: Module<I>): Identifier[] { | ||
| return moduleChildren(m) |
There was a problem hiding this comment.
I would have expected this in syntaxBindings actually.
|
It would be nice to find a more functional-friendly way of writing rules. The visitor patterns rely too much on the execution order and keeping state. It's quite imperative. It would be interesting if we can compare with other implementations at some point. But this looks good, with a bit of clean up and tests we can merge it. It should be easy to maintain. |
2 participants
This needs some improvement around the
Module | Syntaxparts still, as a lot of functions could be merged.