Make ci/lint.dart more idiomatic and move to tools/clang_tidy #26722
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
zanderso
changed the title
zanderso
force-pushed
the
move-linter
branch
4 times, most recently
from
ee4053c to
250ce13
Compare
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
Nice, just a basket of nits. Big picture, looking good.
| _lintAction ??= await _getLintAction(); | ||
|
|
||
|
|
||
| Future<LintAction> _getLintAction() async { |
There was a problem hiding this comment.
nit: this is a good candidate for a pure function that could be made visible for testing.
There was a problem hiding this comment.
I'll give this some thought. Available options for testing the functionality are abstracting the input file stream, providing a fake file system implementation, or writing to the /tmp file system, none of which are very attractive.
There was a problem hiding this comment.
<light hearted rant follows, feel free to ignore it since it isn't a huge deal>
I'm not suggesting writing tests per say, just suggesting that factoring out pure functions (ignoring the filesystem) makes them more testable and by extension more maintainable since they are able to be executed in isolation and the contract of input versus output is more clear than a method (who can touch any instance variable, thus having less encapsulation).
This one was so close to being a static function I thought I'd mention it in my Quixotic quest to make our codebase more Functional.
There was a problem hiding this comment.
It's a good cause. I shuffled things around a bit. I was gloomy at first since I didn't want to abstract out the file system, but I found a way around it.
zanderso
force-pushed
the
move-linter
branch
2 times, most recently
from
796511b to
68712d3
Compare
zanderso
added
the
waiting for tree to go green
3 participants
This PR reorganizes
ci/lint.dartinto a more typical Dart program/library, and adds some tests.