Page MenuHomePhabricator

Add capability to hide lint messages when viewing a diff
Closed, WontfixPublic

Description

We have certain use cases where it would be nice to to be able to hide the messages that show up when viewing a diff. This is usually because of small tweaks to third-party code or legacy code, but the code is stored in locations that it isn't practical to change the entire lint strategy for the project. Sometimes, almost every single line can have a lint error (ie, tab indention instead of spaces), which makes reviewing the diff very difficult.

I feel that it would be very helpful to be able to hide all lint messages when viewing a diff (possibly 3 options - View All, Errors only, None)

For projects/installations that require code to be 100% lint-happy, perhaps the ability to do this should be configurable (on a project level, or global differential settings)

Event Timeline

aripringle raised the priority of this task from to Normal.
aripringle updated the task description. (Show Details)
aripringle added a project: Differential.
aripringle added a subscriber: aripringle.

Although it hasn't fully rolled out yet (and doesn't support everything, and isn't fully documented), .arclint will allow you to easily exclude paths from each linter. Here's an example:

https://github.com/epriestley/arclint-examples/blob/master/.arclint

Generally, I think not raising these messages in the first place is a better approach than having UI to hide them. The "exclude" directive takes a list of regular expressions, e.g.

"exclude" : ["(^externals/)", "(^third-party/)"]

...will stop the configured linter from examining anything in externals/ or third-party/. Would that cover your use case? You can do this with engines today (Phabricator itself excludes its own externals/ directory, for instance), but it's much more involved than .arclint.

Note that if you run with the "NoLintLinter" you can also put @nolint in the body of files like this, but that may not be practical for externals with multiple files.

aripringle claimed this task.

Thanks for the info. It's good to know about .arclint - we use a single lint engine that I'd rather not customize with project-specific paths if I can avoid it.

I think these methods should suffice for the most part. I still think having a way to suppress the messages in the UI would be helpful, although I could also see that making people lazy about dealing with lint issues.

Thanks!

Excluding files seems to me too harsh.
The lint warnings might be useful for the next iteration, but are just very much in the way of reading the code as it is in this revision to be able to comment on other aspects.
I just find myself having to reject the revision and waste a review cycle on linting...
(Also, in some situations fixing the lint issue would make the code inconsistent with the legacy code around it, which is less desirable...)

aftbit added a subscriber: aftbit.

I'm going to reopen this - I frequently experience the same issue as @koreno. It'd be nice to just have a "hide lint warnings" button.

Can you show me a screenshot of an example that demonstrates this problem? It's hard for me to imagine what sort of warnings are valuable enough to raise but not valuable enough to fix.

Please read https://secure.phabricator.com/book/phabcontrib/article/feature_requests/ if you have something you'd like to request for the upstream to build and maintain. There is no need to 'reopen' tickets, as we're capable of doing that ourselves if we feel we will pursue something.

This request is several years old, and I can't come up with a reasonable reason to implement it, both in that it has very, very little impact for the time spent building and maintaining it as well as that it likely points to a larger issue (people are ignoring lint).

There is still valuable feedback that can be given even when there are a
lot of lint errors. Apologies for reopening incorrectly. I'll probably just
make a Firefox extension, since consensus seems to be that this is unneeded
at best.

eadler added a project: Restricted Project.Jul 7 2016, 5:09 PM

When this ticket was re-opened, I lost the ability to close it. If someone else is able to, please feel free.

If you experience this, please file a new task explaining what specific messages you're encountering and why hiding these messages is a better solution than changing the linters to raise more relevant messages. I believe fixing the linters is always a better approach here.