-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add compare-branch option to paver run_quality #6064
Add compare-branch option to paver run_quality #6064
Conversation
Thanks for the pull request, @kluo! I've created OSPR-238 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams. Feel free to add as much of the following information to the ticket:
All technical communication about the code itself will still be done via the Github pull request interface. As a reminder, our process documentation is here. |
@@ -136,6 +137,7 @@ def run_quality(options): | |||
quality of the branch vs master is less than 80%, then this task will fail. | |||
This threshold would be applied to both pep8 and pylint. | |||
""" | |||
compare_branch = getattr(options, 'compare_branch', 'origin/master') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the docstring above this line describes the 'p' parameter. Could you add some doc on this comare-branch param? also, while you're there, you'll need to update some verbage in the 'p' description itself, because it assumes a comparison against master. it will probably make more sense to list the compare-branch param first.
Curious, what if someone passes in a non-existent branch? |
returns
|
d03c80c
to
84f66c0
Compare
Hey @kluo sorry for the delay on my end. It's a relatively small change, but I'd actually prefer it if you took a similar pattern to how the 'percentage' limit is used. That is, only pass in the --compare-branch to diff-quality when it's passed in from paver. When it's not passed in, then the switch is not used on diff-quality, and that uses the diff-quality default (of origin/master). I realize it's not functionally different than what you've already put together---and has not been implemented everywhere like this---but is a better pattern for handling switches that are passed to an underlying utility. For your reference, I mean we should do something like this: I was originally going to do it for you and attach it to this PR (likely with another PR), but it doesn't look like I'll have time to get to that in the next few days, so I'm asking you. I've also been looking at tests in this area. I was originally going to ask you to include tests with your change (another area that we're improving under pavelib), but that is not a small task for the compare-branch change at this point, so I think we can defer that. |
84f66c0
to
218668e
Compare
218668e
to
2262906
Compare
@benpatterson when you have a chance, thanks! |
Looks good, @kluo. Thanks for putting it together. This will be very useful! |
…anch-option Add compare-branch option to paver run_quality
Pretty self explanatory. @benpatterson