Skip to content
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

Feature/diffpatch mission slight cleanup #85

Conversation

paulproteus
Copy link
Contributor

This just performs a tiny amount of cleanup on the diffpatch mission.

@@ -48,8 +49,13 @@ def validate_patch(cls, patchdata):
if len(the_patch.hunks) != 1:
raise IncorrectPatch, 'The patch affects more than one file.'

hunk_set_for_first_file = the_patch.hunks[0]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the second round of this expansion, you don't set a variable. why not use the_patch.hunks[0] over on line 57?

Also, since these blocks are mostly duplicated except for NEW_FILE and OLD_FILE, why not extract to a method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this could be a little cleaner. I'll extract a method and resubmit!

@paulproteus
Copy link
Contributor Author

This version has way better comments, I believe. Review requested!

@pselle
Copy link
Member

pselle commented Nov 9, 2012

👍 Love the addition of the helpful comments (and the extracted method!)

paulproteus added a commit that referenced this pull request Nov 12, 2012
…ght-cleanup

Feature/diffpatch mission slight cleanup

Thanks to pselle for the review, and the comments that led to this improved patch!
@paulproteus paulproteus merged commit 725644a into openhatch:master Nov 12, 2012
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.

None yet

2 participants