refactoring #17
refactoring #17
Conversation
bcpeinhardt
requested review from
TanklesXL and
giacomocavalieri
as code owners
| // Represents information the linter has access to. We want this to include | ||
| // as much as possible and provide ergonomic accessors for querying it. | ||
| // |
There was a problem hiding this comment.
| // Represents information the linter has access to. We want this to include | |
| // as much as possible and provide ergonomic accessors for querying it. | |
| // | |
| /// Represents information the linter has access to. We want this to include | |
| /// as much as possible and provide ergonomic accessors for querying it. | |
| /// |
Let's make this a doc comment
| @@ -1,12 +1,19 @@ | |||
| //// This rule checks the code for unnecessary string literal concatenation, | |||
| //// like "a" <> "b" instead of just "ab". | |||
There was a problem hiding this comment.
| //// like "a" <> "b" instead of just "ab". | |
| //// like `"a" <> "b"` instead of just `"ab"`. |
| fn visit_expressions( | ||
| input: glance.Module, | ||
| rules: List(Rule), | ||
| ) -> List(RuleViolation) { | ||
| let funcs = extract_functions(input) | ||
| let consts = extract_constants(input) | ||
|
|
||
| let f = fn(location_identifier, expr) { |
There was a problem hiding this comment.
What's f? Maybe a more descriptive name could help here
|
I don't think we should transform the rules to constants, because them being functions is a good way to make them configurable. Also the builder pattern brings a lot of niceties, and allows making the rule opaque, which helps with preventing breaking changes, and with simplifying the API. For instance, when having 10 different kinds of visitors, having to specify them all will be very tedious for rule authors. |
|
Cool cool, if y'all wanna keep the builders let reject this PR and then I'll come through and look at some of the naming and flattening callbacks in another PR rather than trying to deal with merge conflicts 😁 |
Did a little refactoring, trying to keep PRs small so going ahead and making one.
I got rid of the builder for rules. I like builders in general for public apis but it meant the ruleset couldn't be constant and we needed helper code for it when we can just construct the type directly internally.
I also renamed RuleError to RuleViolation, I think the word Error should be used for things that are proper errors in our application.
The rest is stylistic work, commenting, using
usea little more, and trying to use some clearer naming patterns.