Skip to content

Add compare-branch option to paver run_quality - #6064

Merged
benpatterson merged 1 commit into
openedx:masterfrom
Stanford-Online:kluo/quality-compare-branch-option
Dec 5, 2014
Merged

Add compare-branch option to paver run_quality#6064
benpatterson merged 1 commit into
openedx:masterfrom
Stanford-Online:kluo/quality-compare-branch-option

Conversation

@kluo

@kluo kluo commented Nov 26, 2014

Copy link
Copy Markdown
Contributor

Pretty self explanatory. @benpatterson

@openedx-webhooks

Copy link
Copy Markdown

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:

  • supporting documentation
  • edx-code email threads
  • timeline information ('this must be merged by XX date', and why that is)
  • partner information ('this is a course on edx.org')
  • any other information that can help Product understand the context for the PR

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.

Comment thread pavelib/quality.py Outdated

Copy link
Copy Markdown
Contributor

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.

@benpatterson

Copy link
Copy Markdown
Contributor

Curious, what if someone passes in a non-existent branch?

@kluo

kluo commented Nov 27, 2014

Copy link
Copy Markdown
Contributor Author
paver run_quality -b nonexistent_branch

returns

diff_cover.git_diff.GitDiffError: fatal: ambiguous argument 'nonexistent_branch...HEAD': unknown revision or path not in the working tree.

@benpatterson

Copy link
Copy Markdown
Contributor

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:
https://github.com/edx/edx-platform/blob/master/pavelib/quality.py#L145

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.

@clytwynec clytwynec added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Dec 3, 2014
@kluo
kluo force-pushed the kluo/quality-compare-branch-option branch from 84f66c0 to 218668e Compare December 4, 2014 20:52
@kluo
kluo force-pushed the kluo/quality-compare-branch-option branch from 218668e to 2262906 Compare December 4, 2014 23:13
@kluo

kluo commented Dec 5, 2014

Copy link
Copy Markdown
Contributor Author

@benpatterson when you have a chance, thanks!

@benpatterson

Copy link
Copy Markdown
Contributor

Looks good, @kluo. Thanks for putting it together. This will be very useful!

benpatterson pushed a commit that referenced this pull request Dec 5, 2014
…anch-option

Add compare-branch option to paver run_quality
@benpatterson
benpatterson merged commit f73d92b into openedx:master Dec 5, 2014
@stvstnfrd
stvstnfrd deleted the kluo/quality-compare-branch-option branch December 19, 2015 02:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants