andygrunwald / go-gerrit

Go client/library for Gerrit Code Review
https://godoc.org/github.com/andygrunwald/go-gerrit
MIT License
96 stars 40 forks source link

Add PatchOptions.Path field. #50

Closed dmitshur closed 6 years ago

dmitshur commented 6 years ago

Gerrit API docs: https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#get-patch.

codecov-io commented 6 years ago

Codecov Report

Merging #50 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #50   +/-   ##
=======================================
  Coverage   21.23%   21.23%           
=======================================
  Files          21       21           
  Lines        1775     1775           
=======================================
  Hits          377      377           
  Misses       1353     1353           
  Partials       45       45
Impacted Files Coverage Δ
changes_revision.go 0% <ø> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8fdb823...3418ea4. Read the comment docs.

andygrunwald commented 6 years ago

LGTM

dmitshur commented 6 years ago

Thanks for the quick review.

I have a handful more of local patches I'll be trying to upstream next. I'll start with the easiest and least disruptive ones first, and leave hardest ones for the end.

@andygrunwald How do you feel about using using the squash merge strategy for merging simple PRs that make one logical change?

image

That way, there's only one entry in the history instead of two, making it more readable and nicer.

andygrunwald commented 6 years ago

I have a handful more of local patches I'll be trying to upstream next. I'll start with the easiest and least disruptive ones first, and leave hardest ones for the end.

This sounds good and I am looking forward to this!

@andygrunwald How do you feel about using using the squash merge strategy for merging simple PRs that make one logical change?

Personally, I don't have a strict opinion about this. I understand the request in terms of having a clean history. For simple PRs this make sense. I would agree with everyone who has a favor for this. But i (personally) don't mind have a Merge commit in. But there i am free what the team (you, @opalmer and i) vote for.

For complex PRs with a couple of commits where every commit has a real use case and an explainable commit message (e.g. https://chris.beams.io/posts/git-commit/), i would disagree to do this, because then the information gets lost.

If you want to go for it, feel free :) I will not block anyone. If you want to get more opinions about this, i suggest to open a new ticket here and mention @opalmer also. There we can discuss this in detail and maybe some other OSS maintainers hook on (i guess it is harder to find this discussion in this PR instead of a separate one)

dmitshur commented 6 years ago

For complex PRs with a couple of commits where every commit has a real use case and an explainable commit message (e.g. https://chris.beams.io/posts/git-commit/), i would disagree to do this, because then the information gets lost.

Oh, I completely agree, I think each logical commit with a clean commit message (rather than just a fixup commit to address review feedback) should be preserved in master. But I would still prefer "rebase and merge" strategy for that over the "merge commit" one.

But what I care more about is just avoiding the merge commit for small, single-logical-commit PRs (such as this one).

andygrunwald commented 6 years ago

Okay, let's try this :)