Add a wrapper around CoffeeLint as an ArcanistExternalLinter.
Depends on D9041.
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Commits
- rARC1493e043e1e0: Add an `ArcanistCoffeeLintLinter` linter.
Wrote and executed unit tests.
Diff Detail
Diff Detail
- Repository
- rARC Arcanist
- Branch
- coffeelint
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 457 Build 457: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
Comment Actions
I'm not sure what the correct way (or if there is even any means to do so) to set .arclint configuration from the *.lint-test file. This is why the unit tests are currently failing.
Comment Actions
Two minor issues, and my guesses on the config thing. I'm open to any approach, but tend to think a full-strength approach (e.g., provide a mechanism for test linters to execute in an arbitrary environment populated with nice config files) is probably not worth it and would be more than satisfied with a "don't bother unit testing it" approach: as long as it works in manual tests, it seems very unlikely to break by surprise (provided coffeelint doesn't, like, ignore unrecognized arguments or consider a bad config file to mean "whatever, ignore it and keep going").
| src/lint/linter/ArcanistCoffeeLintLinter.php | ||
|---|---|---|
| 92–94 | Rebase for type/help, namespace. | |
| 103 | (Leftover debugging code?) | |
| src/lint/linter/__tests__/coffeelint/arrow_spacing.lint-test | ||
| 7–9 ↗ | (On Diff #21492) | I would expect this to work, so if it doesn't that's probably a bug of some kind. A possible issue is that this path may be interpreted relative to the working copy during test execution -- I think? -- not relative to the arcanist root, so it will not exist. I'd expect coffeelint to fatal ("unable to read config file...") but maybe it has some kind of bizarre behavior like ignoring specified but nonexistent config files? This could also just be straight-up bugged. If writing the file is an issue, fixing it is probably messy. Maybe the easiest thing for now is to not try to cover this behavior with tests. |
Comment Actions
I think that if it is easy enough for us to test, then we probably should. If nothing else, it makes sense to provide a range of test cases so that we can easily check if we remain compatible with the latest version of the external linter.
| src/lint/linter/ArcanistCoffeeLintLinter.php | ||
|---|---|---|
| 103 | Yeah, good catch. | |
| src/lint/linter/__tests__/coffeelint/arrow_spacing.lint-test | ||
| 7–9 ↗ | (On Diff #21492) | This was my main motivation for D9067. I believe that ArcanistLinterTestCase was calling setConfig when I was expecting it to call setLinterConfigurationValue. I will have to verify the behaviour again after that diff closed. |
Comment Actions
Ok, so specifying a config file works more than previously, but still ultimately fails because the UnitTestableLintEngine has a temporary working copy. I'll just remove these tests for now.
| src/lint/linter/ArcanistCoffeeLintLinter.php | ||
|---|---|---|
| 38 | It seems somewhat confusing now... we have getInfoName, getLinterName and getLinterConfigurationName. | |
| 54 | This is pretty common... maybe it should be moved to a function somewhere. | |
| 137–149 | This is used in a few places, maybe we should create an ArcanistLintSeverity::getLintSeverityFromString method. | |
Comment Actions
Closed by commit rARC1493e043e1e0 (authored by @joshuaspence, committed by @epriestley).