Skip to content

Conversation

@taufek
Copy link
Contributor

@taufek taufek commented Feb 5, 2018

Continuation from #536 where it added modified_files for PrePush context.
This is to add modified_lines_in_file for PrePush context.

Copy link
Owner

@sds sds left a comment

Choose a reason for hiding this comment

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

Overall looks great, @taufek. Thanks for writing thorough tests.

Once a few small comments are addressed I'll be happy to merge. Thanks!

@modified_lines[file]
end

def refs
Copy link
Owner

Choose a reason for hiding this comment

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

Let's use the nomenclature in the Git documentation and call this ref_range. I'm concerned that in this context refs is a bit too vague for folks looking at this code for the first time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed.

Overcommit::GitRepo.extract_modified_lines(file, refs: refs)
end

@modified_lines[file]
Copy link
Owner

Choose a reason for hiding this comment

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

You can shorten this method implementation to:

@modified_lines ||= {}
@modified_lines[file] ||= Overcommit::GitRepo.extract_modified_lines(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome.

Continuation from sds#536 where it added `modified_files` for `PrePush` context.
This is to add `modified_lines_in_file` for `PrePush` context.
@taufek taufek force-pushed the tj-pre-push-mod-file-lines branch from 812bbb8 to 664ff82 Compare February 5, 2018 22:04
@taufek
Copy link
Contributor Author

taufek commented Feb 5, 2018

@sds . Thanks for the feedback. This is ready for re-review.

@sds sds added the enhancement label Feb 5, 2018
@sds sds merged commit 2594697 into sds:master Feb 5, 2018
@taufek taufek deleted the tj-pre-push-mod-file-lines branch February 7, 2018 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants