Page MenuHomePhabricator

Add a top-level mechanism for disabling lint by adding a comment
Open, NormalPublic

Description

While I'm not thrilled about this practice and unconvinced it's ever the right approach (see discussion on "expert teams" in T1344), it's widespread in other lint infrastructure and not wildly unreasonable. Basically, allow a comment on a line to suppress specific lint warnings emitted on that line:

$committ_messsage = "..."; // @suppress SPELL1

...with some comment text like "@suppress", @arc-no-lint or similar.

Event Timeline

epriestley triaged this task as Normal priority.Jul 24 2012, 7:31 PM
epriestley added a project: Arcanist.
epriestley added subscribers: epriestley, cpiro.

We have external code in the facebook phabricator repository (it's the PHP Thrift stuff) that fails the PHL3 "One Class Per File" test. I got a bunch of warnings abot it on an unrelated diff*. It would be nice to have a way to disable lint for that and anything else in src/externals.

  • I don't think we want to edit all of the Thrift PHP code to add @arc-no-lint-in-file or somesuch everywhere, that seems to go against the spirit of copying and pasting other people's code into your repository without editing it in any way.
  • I'm not a huge fan of using files-as-configuration (e.g. having a flag file like src/externals/.arc-no-lint-below-here), though I've seen that done enough in other projects.
  • .arcconfig might be a good place to add configuration options to the linter?

Maybe the PHL3 error is there for a good reason and we should actually just fix that instead? :D

*https://phabricator.fb.com/D554622

I think that case is well-covered by existing stuff:

http://www.phabricator.com/docs/phabricator/article/Arcanist_User_Guide_Customizing_Existing_Linters.html

Specifically:

  • Create two linters, one configured for src/externals (which disables PHL3 and whatever else isn't relevant) and one configured for everything else.
  • Run the externals linter on src/externals.
  • Run the normal linter on everything else.

I'm very hesitant to move this stuff into configuration because I'm worried configuration isn't powerful enough to reasonably express the number of custom lint configurations projects have.

Cool. Let me know if you hit issues, but I think it should be pretty straightforward given that you already have custom code.

This might be slightly harder in that ArcanistPhutilLibraryLinter is running phutil_rebuild_map.php and reporting all errors, even if the errors aren't in files added to the linter with ->addPath().

Ah. We probably need to add a startup event or something that lets you include or register autoloaders for externals, so you can move the externals out of your libphutil library. You could just hackily put this in __phutil_library_init__.php for now, i.e.

yourlibrary/
  externals/   # thrift, etc
  src/         # actual libphutil library
    __phutil_library_init__.php # add include_once '../externals/thrift/Autoloader.php'; to the bottom
    blah/
    etc/
chad changed the visibility from "All Users" to "Public (No Login Required)".Jul 3 2015, 4:08 AM