Page MenuHomePhabricator

Added support for `export LC_ALL=C.UTF-8` to the shell linter
ClosedPublic

Authored by jasonbcox on Sep 30 2019, 23:04.

Details

Summary
Test Plan

arc lint on the following cases:

  • script with export LC_ALL=C
  • script with no LC_ALL set. Fails as expected.
  • script with export LC_ALL=C.UTF-8
  • script with export LC_ALL=C.UTF-9 Fails as expected (sanity check that we don't accidentally match on export LC_ALL=C)

Diff Detail

Repository
rABC Bitcoin ABC
Branch
lc-all
Lint
Lint Errors
SeverityLocationCodeMessage
Errorarcanist/linter/ShellLocaleLinter.php:1PHPCS.E.Generic.PHP.RequireStrictTypes.MissingDeclarationGeneric.PHP.RequireStrictTypes.MissingDeclaration
Unit
No Test Coverage
Build Status
Buildable 7638
Build 13316: Bitcoin ABC Buildbot (legacy)
Build 13315: arc lint + arc unit

Event Timeline

Fabien requested changes to this revision.Oct 1 2019, 06:31
Fabien added a subscriber: Fabien.
Fabien added inline comments.
arcanist/linter/ShellLocaleLinter.php
1

You should have a very recent phpcs because this rule is 3 months old according to github:
https://github.com/squizlabs/PHP_CodeSniffer/blob/master/src/Standards/Generic/Sniffs/PHP/RequireStrictTypesSniff.php

It enforces using the new PHP 7 strict_types declaration: https://www.php.net/manual/fr/control-structures.declare.php.

Because none of our code nor arcanist code is compliant, an exception can be added to the rule file (arcanist/phpcs.xml).

61

PHP has the array_search() built-in for doing this:

if (array_search(trim($matches[0][0]), self::LOCALE_STATEMENTS, true) !== false) {
  return $this->raiseLintAtPath(
      self::INVALID_LOCALE,
      pht('Shell scripts should set the locale to avoid side effects.')
    );
}
This revision now requires changes to proceed.Oct 1 2019, 06:31

Maybe there should be an explicit notice of backport here, as the remaining parts of the PR are Travis related ?

jasonbcox added inline comments.
arcanist/linter/ShellLocaleLinter.php
1
jasonbcox added inline comments.
arcanist/linter/ShellLocaleLinter.php
61

cool. equality is flipped though ;)

jasonbcox marked an inline comment as done.

Rebase + Fix according to feedback

Fabien added inline comments.
arcanist/linter/ShellLocaleLinter.php
61

Oops... good catch !

This revision is now accepted and ready to land.Oct 1 2019, 18:47