Page MenuHomePhabricator

[cashtab] add arcanist linter support for eslint
AbandonedPublic

Authored by majcosta on Apr 30 2021, 18:02.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Summary

basic eslint recommended settings configuration and a helpful .arclint from https://secure.phabricator.com/D12198

it does static analysis, catches js errors and is generally well regarded. we already have it as a devDependency for cashtab so we might as well run it and integrate with phab

Test Plan

arc diff this patch touching one file with a warning and a file with errors

fail to submit the patch if the errors aren't fixed see all linter warnings and errors in the diff body

Diff Detail

Repository
rABC Bitcoin ABC
Branch
add_arc_lint_eslint
Lint
Lint Errors
Unit
No Test Coverage
Build Status
Buildable 15609
Build 31134: Build Diffcashtab-tests
Build 31133: arc lint + arc unit

Event Timeline

revert the linter complaints and green the diff

Fabien requested changes to this revision.May 3 2021, 06:43
Fabien added a subscriber: Fabien.
Fabien added inline comments.
.arclint
297 ↗(On Diff #28319)

This is probably not doing what you expect. I suppose you meant (\\.jsx?$) ?

298 ↗(On Diff #28319)

If it's empty you'd better remove it

299 ↗(On Diff #28319)

What is the $0 here ?

There are a few reasons why I don't like script-and-regex, and here are some:

  • There is no version checking
  • There is no guarantee this will work on all platforms (does it run on macOS for example ?)
  • Performance suffers from calling a shell for each file to process. This has proven to be a problem in the past. In this case it's even worst: PHP is calling a shell, which calls a shell, which calls two shells (see your parenthesis), one of them ending up calling eslint.

I don't see a reason why this could not be a plain PHP linter that solves all of these issues. Let me know if you need help for writing it.

This revision now requires changes to proceed.May 3 2021, 06:43