Skip to content

Pylint: ignore TODO comments - #5814

Merged
singingwolfboy merged 3 commits into
masterfrom
disable-pylint-todo-comments
Nov 12, 2014
Merged

Pylint: ignore TODO comments#5814
singingwolfboy merged 3 commits into
masterfrom
disable-pylint-todo-comments

Conversation

@singingwolfboy

Copy link
Copy Markdown
Contributor

TODO comments are currently flagged by pylint, which causes them to break the build on Jenkins. We have many TODO comments already in the codebase, and it isn't practical to stop working to fix them all. We also don't want to just delete them: they contain valuable information. However, anytime that code is touched, diff-quality flags the TODO and fails the build.

It would be nice if there were a way to have diff-quality report on these TODO comments, but more as a warning, and not have them fail the build. However, I don't know how to do that, so this is the next best solution. TODO comments and similar can always be caught by humans in code review.

@singingwolfboy

Copy link
Copy Markdown
Contributor Author

@andy-armstrong @jzoldak: you two are probably interested in having a discussion on this pull request.

@jzoldak

jzoldak commented Nov 3, 2014

Copy link
Copy Markdown
Contributor

This is TE-564
I think a better way to go about it is the approach outlined there, namely to see if we can change the paver run_quality command to add --disable=W0511

@singingwolfboy

Copy link
Copy Markdown
Contributor Author

@jzoldak That's fine with me. I've changed it in this PR. Can I get a thumbs-up?

@jzoldak

jzoldak commented Nov 4, 2014

Copy link
Copy Markdown
Contributor

@singingwolfboy did you test that it works as expected?

@singingwolfboy

Copy link
Copy Markdown
Contributor Author

I was editing the wrong task -- thanks for pointing that out, @jzoldak. I've added it to run_quality and removed it from run_pylint. (Or do we want it on run_pylint too?) I've tested it on devstack, and it seems to work correctly.

@singingwolfboy
singingwolfboy force-pushed the disable-pylint-todo-comments branch from 79aabb7 to 9902dcb Compare November 6, 2014 14:32
@andy-armstrong

Copy link
Copy Markdown
Contributor

👍 Thanks for tackling this, @singingwolfboy.

@benpatterson

Copy link
Copy Markdown
Contributor

LGTM 👍

singingwolfboy added a commit that referenced this pull request Nov 12, 2014
@singingwolfboy
singingwolfboy merged commit d981f98 into master Nov 12, 2014
@singingwolfboy
singingwolfboy deleted the disable-pylint-todo-comments branch November 12, 2014 15:51
@benpatterson

Copy link
Copy Markdown
Contributor

When you merge, could you put TE-564 in the commit message?

@benpatterson

Copy link
Copy Markdown
Contributor

Ah you are too quick. No big deal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants