Page MenuHomePhabricator

Run script-and-regex linter on all files
AbandonedPublic

Authored by bgamari on Nov 22 2015, 8:05 PM.

Details

Summary

The documentation vaguely suggests that this is the intended behavior.

Test Plan

None

Diff Detail

Repository
rARC Arcanist
Lint
Lint OK
Unit
Unit Tests OK
Build Status
Buildable 8973
Build 10534: arc lint + arc unit

Event Timeline

bgamari updated this revision to Diff 35162.Nov 22 2015, 8:05 PM
bgamari retitled this revision from to Run script-and-regex linter on all files.
bgamari updated this object.
bgamari edited the test plan for this revision. (Show Details)
epriestley requested changes to this revision.Nov 22 2015, 9:04 PM
epriestley added a reviewer: epriestley.

Please sign the CLA (L28 or L30) before contributing to Phabricator.

This revision now requires changes to proceed.Nov 22 2015, 9:04 PM

@csilvers, I think you are the flagship user of this binding -- are these changes going to break you?

The documentation vaguely suggests that this is the intended behavior.

I'd probably didn't intend this, can you point me at where things are ambiguous in your reading? We can clarify the docs, at least.

I think the default behavior of not linting this stuff is mostly-reasonable and probably correct -- many linters do not work correctly when run as linter file-that-does-not-exist or linter directory, and these cases are difficult to exclude with .arclint rules. I'm tentatively OK with adding a script-and-regex.lint-everything configuration option which defaults to false but causes the behavioral changes here when enabled, although this is a bit of a slippery slope toward script-and-regex.lint-just-this-subset-of-things which I'm less-than-enamored by the prospect of. But I'm OK with an option + a documentation update.

The documentation contains this paragraph,

Note that when run via arc diff, the list of files to be linted includes deleted files and files that were moved away by the change. The linter should not assume the path it is given exists, and it is not an error for the linter to be invoked with paths which are no longer there. (Every affected path is subject to lint because some linters may raise errors in other files when a file is removed, or raise an error about its removal.)

I liberally read this to mean that the linter should be run against all files. Indeed clarifying the documentation here would be quite useful; I killed quite a long time today trying to work out why my linter wasn't getting run.

However, at this point I have no need for this change as we'll be using the extension originally proposed for merging in D14535.

Ah, thanks! I'll fix the docs, I think they probably predate us breaking out these special filetypes (long ago, every linter needed to account for them, but most linters did not and could not easily be made to, and it was easy to not test these special cases, and very few linters care about deleted files anyway).

If you don't need this anymore, let's just leave things as they are.

bgamari abandoned this revision.Nov 22 2015, 11:24 PM

Sounds good to me. Thanks again!

Sounds like this ended up having no changes? Let me know if you still need input from me for anything.

Yep, you aren't affected -- just one documentation change. Thanks for following up!