Skip to content

Conversation

@baywet
Copy link
Member

@baywet baywet commented Sep 14, 2020

Fixes #357

Warning: this pull request contains #463 to ease up the writing of the unit test. 463 should be reviewed and merged first.

Changes proposed in this pull request

Other links

@baywet baywet self-assigned this Sep 14, 2020
@baywet baywet added this to the 2.2.0 milestone Sep 14, 2020
@baywet baywet requested a review from nikithauc September 14, 2020 18:10
@zengin
Copy link
Contributor

zengin commented Sep 14, 2020

@baywet, is this the only commit that requires review here: 73a632b?

@baywet
Copy link
Member Author

baywet commented Sep 15, 2020

@zengin yes, and sorry for the mess, I needed the other PR to build my unit tests

@zengin
Copy link
Contributor

zengin commented Sep 15, 2020

@baywet, in these cases, rebasing the branch could help clear the diff. If that doesn't do the job, creating a new feature branch and cherry-picking relevant commits over can simplify the whole process. Then existing PR can be closed with a link to the new one.

Doing this has two benefits:

  1. Short term: saving time in code reviews, especially when there are multiple repositories and cost of context switching is high.
  2. Long term: revisiting old PRs, whether it is through "git bisect" or just a casual linear look over the merged PRs, will be cheap as well because the actual change is a much smaller subset. In this particular PR, we have the history due to our notes to the relevant commits, but if the reviewer knows it by other means (e.g. the discussion happened elsewhere), the context will be lost from official Github history.

Of course, everything has a trade-off and it may not be feasible to do that in certain situations. But I think, in general, the benefits for reviewers and everybody who looks at the history at a later time, including the author, outweigh the trouble of moving things around.

My two cents. Please let me know what you think.

@baywet baywet force-pushed the bugfix/content-type-post-override branch from e2cefa6 to 4057b68 Compare September 15, 2020 14:49
@baywet
Copy link
Member Author

baywet commented Sep 15, 2020

@zengin makes perfect sense! I'm always hesitant to force push and/or rebase in repos where it's not a common practice. It usually confuses people. But if the team is ok with that I'll use it as common practice.
I just rebased the branch which should make the review much easier :)

@zengin
Copy link
Contributor

zengin commented Sep 15, 2020

@baywet I agree that force pushes shouldn't be common practice, especially on the main branches. But throw-away feature branches should be OK to be rebased. I see no downside to rebasing if the PR hasn't been created yet or reviews hasn't started. The latter one is hard to check because once the PR is created, the review may get started without any indication in the Github interface :)

zengin
zengin previously approved these changes Sep 15, 2020
@baywet baywet merged commit 74b0a4f into dev Sep 15, 2020
@baywet baywet deleted the bugfix/content-type-post-override branch September 15, 2020 18:38
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.

Empty POST request rewrites content type

3 participants