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

Latest json pr review #1

Merged
merged 8 commits into from
May 18, 2021
Merged

Latest json pr review #1

merged 8 commits into from
May 18, 2021

Conversation

bskinn
Copy link
Owner

@bskinn bskinn commented May 17, 2021

Merge changes from Apr 2021 @ewjoachim reviews.

No failures introduced to test suite on the change.
JSON API views now directly return, rather than redirecting.

Per
pypi#8615 (review)
For completeness, and to satisfy 100% coverage requirement.
Addresses pypi#8615 (comment).

The database queries inline here may be duplicative; will check
in a subsequent commit.
The 'latest' members off of the Project model now return
fully-realized Release instances, making a subsequent database
call unnecessary.
@bskinn bskinn marked this pull request as ready for review May 17, 2021 18:25
@ewjoachim
Copy link

ewjoachim commented May 18, 2021

(I'm not sure if you expect me to review this PR or if we continue the work on the main PR)
(Very nice work, btw)

@bskinn
Copy link
Owner Author

bskinn commented May 18, 2021

@ewjoachim Thanks!

I thought that setting it up this way might provide cleaner diffs, with this PR showing just the changes I'd made per your reviews. Not sure if it's a standard workflow or not.

In any event, I don't have a preference for whether review happens here or back on the main PR... I could see it making more sense to have the review on the main PR, though, so that all the review is collected in one place. If you agree, I'll go ahead and just merge this PR so that the changes propagate back to pypa#8615.

@ewjoachim
Copy link

ewjoachim commented May 18, 2021

No problem, let's do that on the main PR.

A few "classical" ways to solve this:

  • Push additional commits to the main PR, given one can review commit by commit and GitHub will even tell you which commits are new since your review. If you want to be very explicit, add a comment in the PR stating the name of the first commit to review.
  • If you really want a clean history, you can create "fixup commits" with git commit --fixup <commit hash> so that you can push them now, and once they've been reviewed, you can automatically clean your history with git rebase --autosquash upstream/main. Note that, on Warehouse, spending time cleaning the history is not necessarily useful given the maintaniers usually squash and merge, so all commits get squashed into a single one.

So all in all, don't bother too much, stacking git commits in a single PR is usually enough even when there are multiple rounds of review.

@bskinn bskinn merged commit 58b13aa into latest-json May 18, 2021
@bskinn bskinn deleted the latest-json-pr-review branch May 18, 2021 15:04
@bskinn
Copy link
Owner Author

bskinn commented May 18, 2021

Ahh, I hadn't fully realized how --fixup and --autosquash worked together -- cool! Definitely will be useful in some situations; and, I agree, definitely limited utility there for projects with a squash-merge PR policy. 👍

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.

2 participants