Skip to content

Add Verions Owner Actions to the API #1947

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 19, 2019
Merged

Add Verions Owner Actions to the API #1947

merged 1 commit into from
Dec 19, 2019

Conversation

markcatley
Copy link
Contributor

@markcatley markcatley commented Dec 11, 2019 •
edited by carols10cents
Loading

Contributes to Issue #1548

@rust-highfive
Copy link

r? @sgrif

(rust_highfive has picked a reviewer for you, use r? to override)

@markcatley
Copy link
Contributor Author

r? @carols10cents

Thanks for fixing up and merging my previous PR. Here is the next step of Issue 1548.

@rust-highfive rust-highfive assigned carols10cents and unassigned sgrif Dec 11, 2019
carols10cents
carols10cents requested changes Dec 13, 2019
View reviewed changes
Copy link
Member

@carols10cents carols10cents left a comment

Choose a reason for hiding this comment

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

I tried this out locally and it works great!! 🎉

There are just a few comments in the tests that can be removed now, and some of the test expectations need to be updated so that they don't rely on the ordering of the audit actions.

Thanks!

@markcatley markcatley force-pushed the version-owner-actions-view branch 2 times, most recently from 06c8c0a to 314016c Compare December 13, 2019 23:35
carols10cents
carols10cents requested changes Dec 18, 2019
View reviewed changes
Copy link
Member

@carols10cents carols10cents left a comment

Choose a reason for hiding this comment

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

Hm so looking at all the places you needed to add the ordering makes me think we should extract a function to keep all this logic in one place so that we don't miss a spot if we need to update it in the future. Something like VersionOwnerAction::all_for_versions that would take a slice of versions?

Sorry for adding One More Thing... are you up for making that change? If not, let me know and I'm happy to take care of it :)

@markcatley markcatley force-pushed the version-owner-actions-view branch from 314016c to 88f8aaf Compare December 18, 2019 21:44
@carols10cents
Copy link
Member

Looks great now!! Thank you so much!!

@bors r+

@bors
Copy link
Contributor

bors commented Dec 19, 2019

📌 Commit 88f8aaf has been approved by carols10cents

@bors
Copy link
Contributor

bors commented Dec 19, 2019

⌛ Testing commit 88f8aaf with merge 1608869...

bors added a commit that referenced this pull request Dec 19, 2019
@bors
Auto merge of #1947 - markcatley:version-owner-actions-view, r=carols…
1608869 
…10cents

Add Verions Owner Actions to the API

Contributes to Issue #1548
@bors
Copy link
Contributor

bors commented Dec 19, 2019

☀️ Test successful - checks-travis
Approved by: carols10cents
Pushing 1608869 to master...

@bors bors merged commit 88f8aaf into rust-lang:master Dec 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carols10cents carols10cents carols10cents approved these changes

Assignees

@carols10cents carols10cents

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@markcatley @rust-highfive @carols10cents @bors @sgrif