Clytwynec/dont fail diff quality for todo

[clytwynec]

No description provided.

[clytwynec]

@benpatterson @jzoldak Please review

[clytwynec]

FYI: https://jenkins.testeng.edx.org/job/edx-all-tests-auto-pr/18993/ is the build with these changes along with a new TODO. I've since removed the TODO commit and rebased to fix the unit test failures.

[jzoldak]

I also just cherry-picked your commits onto my PR #6111 which was failing the quality shard due to commits.

[benpatterson]

If we're disabling this for the pylint runs, do we still need to fix run_quality (the flag disabling this rule) ? If someone were to introduce a TODO, would diff-quality find it and error on it?

[clytwynec]

@benpatterson diff-quality didn't fail when I tested adding a new TODO.

I think that is because it is using the files generated in the run_pylint step. I'm not sure how it will play out if you only use run_quality though (without running pylint like we do in all-tests.sh first).

Looking at this comment ...

    # If pylint reports exist, use those
    # Otherwise, `diff-quality` will call pylint itself

... my guess is it would fail because we are not passing the disable flag to diff-quality. I'll look into whether or not that requires a change to diff-quality.

[benpatterson]

Well, I mainly bring it up because -- I thought -- the way we had diff-quality set up had a bug, where the 'disable' option wasn't being recognized or used correctly...

[clytwynec]

Ok. So after a bit more investigating, it looks like the --disable=fixme option in run_quality works. The issue is that diff-quality only runs pylint if it doesn't find existing reports. Since we are using run_pylint before run_quality in all-tests.sh and run_pylint wasn't disabling fixme, diff-quality was checking against reports that count TODOs.

So, with this change, if one uses run_quality locally (having not already generated any reports, it will use disable fixme. However, if you use run_pylint first, without passing -d fixme, it will not disable fixme the next time you use run_quality as it is using the previously generated files.

[benpatterson]

Ahhh... Ok, that makes sense.

I know you put work into this, but in this case, would we be better off just globally disabling fixme in the pylintrc? If we have to disable it in both pylint runs anyway with the switches....

[clytwynec]

I think that makes sense... though we may want to add a way for people to explicitly find TODOs.

[benpatterson]

@sarina @singingwolfboy care to weigh in?

[sarina]

I am fine with pretty much any approach. If we end up putting fixme disable in pylintrc, I might want to see a script that will print out all the fixme violations just so we can easily understand what that tech debt is (and perhaps, monitor it)

[singingwolfboy]

Please use the violation name (fixme) rather than the violation code (W0511) -- names are much clearer.

[sarina]

@clytwynec - can we get this merged?

[clytwynec]

Waiting on re-review. @benpatterson @jzoldak

[benpatterson]

@clytwynec I have some comments/questions about the find_fixme script. Would it be possible to split that out into a separate PR so we can get the pylintrc changes in?

Otherwise I can comment here...thought I'd stop and ask first.

[clytwynec]

@benpatterson done. See https://github.com/edx/edx-platform/pull/6173.

[benpatterson]

Right on.

👍 on this change. I'll look at the other PR as well.