I don't understand the difference between this and T9936. Aren't they aiming to solve the same problem (difficulty remembering what failed during arc diff when running arc land)?

(Also, I think neither this nor T9936 describe bugs in Phabricator. See Contributing Feature Requests and particularly Describing Root Problems.)

Oh -- are you creating these from the workboard? Never mind, in that case.

(See general churn in T9905, T9908, T9132.)

I saw it as two subproblems

1. Difficulty remembering whether lint or unit failed
2. Difficulty remembering excuse for lint failures in particular.

Stepping back, they can both be combined into that one larger issue (can't remember what happened at arc diff when typing arc land). This typically happens for me when several days pass between "arc diff" and "arc land".

Here is a scenario that has bit me in particular when working on a diff that touched many files.

1. arc diff. Make legitimate lint excuse. Make a bogus unit excuse like "These unit tests take a while. I'll run it on the final revision"
2. Put up for code review. Several days pass. It gets accepted
3. arc land. Skip through the warning because I know there was a legitimate lint error and misremembered my running of unit.

In this case for me, the reasons for bypassing lint/unit failures are at the front of the mind during "arc diff", but in distant memory by "arc land".

Our install does have a separate issue which exacerbates this issue that our lint warns too often, but we're working through that separately. Regardless, I do think some tooling assistance is helpful.

It seems to me that the root purpose of the "Land revision anyway" warning is to remind you of what your earlier failures, so making that reminder more explicit seems handy.

I'm going to go ahead and merge in T9936

These unit tests take a while. I'll run it on the final revision

• Specifically, you're passing --nounit?
• Don't you have builds of revisions hooked up so it's sort of moot if you run tests or not? Maybe not for this project? Or is that not working well?
• Can you make the tests run fast instead of slow?
• Can you delete every linter which emits false positives right now? I strongly recommend this and I think I suggested it in at least once or twice in March. If they're still flaky 9 months later, maybe it's more politically reasonable to delete them all now?

1. In this case I was yes.
2. We have post-push builds hooked up, but not pre-push for this project in particular. (rely on arc unit)
3. We have tests only run on affected files, but we have certain files that can trigger a lot of tests.
4. I'm all for this (reducing some big categories from warning to advice by default). Going to try to make it happen. Would be cool to have a list of "top errors in this project" somewhere to make this process easier.

4 is a bit tricky because we use regex matching on pep8 warnings, but we also have multiple big old nasty codebase which wasn't pep8 from the start, but I'm going to try to pull the regex down to advice and pull a few select ones up (which we've sufficiently fixed to warrant) to warning (in the particular project I'm working on).

I still think that a reminder would be handy since my memory is bad, but I can report back after dropping the hammer on these warnings.

I was able to get some metrics for how many revisions have lint errors in this particular project (repo callsign 'C') with this query:

/* Lint status for all latest diffs */
SELECT
  lintStatus,
  COUNT(*)
FROM (
  SELECT
     phabricator_differential.differential_diff.revisionID,
     phabricator_differential.differential_diff.lintStatus,
     MAX(phabricator_differential.differential_diff.dateModified)
   FROM
     phabricator_differential.differential_diff
     JOIN phabricator_repository.repository
     ON phabricator_repository.repository.phid = phabricator_differential.differential_diff.repositoryPHID
   WHERE
     phabricator_repository.repository.callsign = 'C' AND
     phabricator_differential.differential_diff.dateCreated > (1449701043 - (3600 * 24 * 7)) AND
     phabricator_differential.differential_diff.creationMethod = 'arc'
   GROUP BY
     phabricator_differential.differential_diff.revisionID,
     phabricator_differential.differential_diff.lintStatus
) one_per_revision
GROUP BY lintStatus

+------------+----------+
| lintStatus | COUNT(*) |
+------------+----------+
|          0 |        2 |
|          1 |       65 |
|          2 |       19 |
|          3 |       13 |
|          4 |        2 |
+------------+----------+

I've made some big changes to our lint rules to hopefully largely reduce the lint false positives on this particular project and I'll be following along with that metric and hopefully see a lot more "1"s.

I do think it is several clicks from the "are you sure you want to land despite lint/unit failures" to determine what you clicking through.
It roughly sounds like we don't want to optimize the "failed lint" workflow to be easier to navigate because it encourages bad linting setups. In that case, we should close this one out as WONTFIX.

Thanks for the timely responses!

That prioritization makes sense to me. It doesn't seem worth putting a lot of effort into optimizing the workflow for a bad configuration.

Makes more sense to make it easy to convert bad configurations into good ones.

I do in particular think that tools for understanding effects of lint across a project would be really enabling. Having worked on several projects over here, over-aggressive lint configurations appear a chronic problem. I believe the failure mode arises with:

1. Have an old codebase
2. Add a large number of lint rules (pylint for example) and use default severities
3. Many people ignore severities due to existing code errors / false positives.
4. Very little insight into # of false positives.

For now, I'm going to use this query to try to improve the situation on this one project by sorting priorities by hand on the lint messages we hit most frequently. It will also help us prioritize which sorts of warnings we should go back and one-time fix in our codebase. Thanks!