Closed calda closed 3 months ago
Attention: Patch coverage is 98.30508%
with 4 lines
in your changes are missing coverage. Please review.
Project coverage is 95.17%. Comparing base (
ca70962
) to head (532eab8
).
Files | Patch % | Lines |
---|---|---|
Sources/Rules.swift | 98.31% | 3 Missing :warning: |
Sources/ParsingHelpers.swift | 97.95% | 1 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@nicklockwood, I updated the rule implementation to support both prefer-locals-only
and explicit
rather than just inferred
like it did before. This PR also includes a bunch of bug fixes. I added some context and commentary to the PR description, let me know what you think.
We may also want to rename the --redundantType
option to --propertyType
and then have both the redundantType
and propertyType
rule respect it. It's slightly weird for the propertyType
rule to use the --redundantType
option, but I agree your thinking that we do want them to be configured using the same option rather than two separate options (so there's never a conflict). I'm also fine with leaving it as-is.
@calda thanks so much for these features and fixes, I really appreciate it. Sorry I've not had very much bandwidth to discuss the design. I'm not certain yet what the best approach is with respect to naming. I'll try to collate my thoughts and get back to you in the next few days.
Thanks, no worries :) appreciate the merges, we can iterate on develop
This PR renames the
preferInferredTypes
rule topropertyType
, and makes the rule support both directions of conversions. It now fully respects the--redundantType inferred/explicit/infer-locals-only
option, and can convert freely betweenand
That being said each option has various edge cases that may require manual fixes. When I ran these on Airbnb's app codebase:
inferred
had about 15-20 cases that I had to fix manually (not so bad considering we have like 2M+ LOC!)infer-locals-only
mostly only had issues with cases where we had functions that look like types (e.g.func Translation(...)
) or static members that don't return the same type. I added a--preservesymbols
option that lets you exclude any sort of type or property that has this issue, which solved most of these issues. After this it was only miscellaneous issues that were individually easy to work around.explicit
, but it seems reasonable to support it if somebody wants to use it. I expect this would be somewhat difficult, but it's definitely possible.I'd be fine with leaving the rule as-is (
preferInferredTypes
only supportinginferred
rather thanexplicit
orinfer-locals-only
) if you think theinfer-locals-only
andexplicit
options are too unreliable. I findinferred
andinfer-locals-only
definitely reliably enough for use in Airbnb's codebase / style guide.This PR also fixes other issues where the existing code would cause build failures:
For example, this compiles:
but this doesn't:
I would have posted the two changes as separate PRs, but the
propertyType
rewrite depended on the bugfixes.