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.

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

Modify context key type #10375

Merged
merged 2 commits into from
Feb 11, 2019
Merged

Modify context key type #10375

merged 2 commits into from
Feb 11, 2019

Conversation

KimMachineGun
Copy link

I think that define type with struct{} and using it as a context key is more go-ish.

And, actually golang/go project has been using it.

@ghost ghost added the proposed label Oct 16, 2018
@mark-rushakoff mark-rushakoff changed the base branch from master to 1.8 January 11, 2019 19:02
e-dard
e-dard approved these changes Feb 1, 2019
View reviewed changes
Copy link
Contributor

@e-dard e-dard left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@e-dard e-dard requested a review from stuartcarnie February 1, 2019 16:00
@e-dard e-dard added the community-pr Pull requests from the community. Thank you! label Feb 1, 2019
@stuartcarnie
Copy link
Contributor

@KimMachineGun could you sign the CLA

@KimMachineGun
Copy link
Author

KimMachineGun commented Feb 2, 2019 •
edited
Loading

@stuartcarnie sure 😊

@e-dard
Copy link
Contributor

e-dard commented Feb 4, 2019

@KimMachineGun Thanks for you contribution. As well as signing the CLA you will need to fix the conflict in context.go.

e-dard
e-dard previously requested changes Feb 4, 2019
View reviewed changes
Copy link
Contributor

@e-dard e-dard left a comment

Choose a reason for hiding this comment

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

Awaiting CLA agreement

@e-dard e-dard dismissed their stale review February 4, 2019 11:43

CLA is now signed.

stuartcarnie
stuartcarnie approved these changes Feb 5, 2019
View reviewed changes
Copy link
Contributor

@stuartcarnie stuartcarnie left a comment

Choose a reason for hiding this comment

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

Thanks for the PR 👍

@e-dard e-dard merged commit 8b5b0fc into influxdata:1.8 Feb 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stuartcarnie stuartcarnie stuartcarnie approved these changes

@e-dard e-dard e-dard left review comments

Assignees
No one assigned
Labels
community-pr Pull requests from the community. Thank you! proposed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@KimMachineGun @stuartcarnie @e-dard