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

Remove metautils functions that call FromContext and NewContext #38

Merged
merged 1 commit into from
Aug 25, 2017
Merged

Conversation

bufdev
Copy link
Contributor

@bufdev bufdev commented May 4, 2017

See grpc/grpc-go#1219, grpc/grpc-go#1178, grpc/grpc-go#1157, grpc/grpc-go#1148

This code isn't used within the repository, and godoc imports says it isn't used anywhere else it can detect (although someone might be using it privately). With the new API changes, this is a bug waiting to happen, see grpc-ecosystem/grpc-opentracing#29. I think removing these two functions might be the best call for now (better to have a compile-time error from a consumer than a runtime issue).

@codecov-io
Copy link

codecov-io commented May 4, 2017

Codecov Report

Merging #38 into master will increase coverage by 0.83%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #38      +/-   ##
==========================================
+ Coverage   74.24%   75.08%   +0.83%     
==========================================
  Files          34       34              
  Lines        1223     1204      -19     
==========================================
- Hits          908      904       -4     
+ Misses        267      252      -15     
  Partials       48       48
Impacted Files Coverage Δ
util/metautils/single_key.go 50% <ø> (+22%) ⬆️

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 f63a7df...77a0d06. Read the comment docs.

@mwitkow
Copy link
Member

mwitkow commented May 6, 2017

Hmm, can this wait until they actually nuke it?

@bufdev
Copy link
Contributor Author

bufdev commented May 6, 2017

I don't think they actually will remove the methods so that there is backwards compatibility, they really shouldn't remove them until 2.0. Note that right now, GetValue(SetValue(ctx, "foo", "bar"), "foo") will not work, which could be a bug just waiting to happen. This is related to grpc/grpc-go#1219

@mwitkow
Copy link
Member

mwitkow commented Jun 11, 2017

Can you please resolve the conflicts and I'll merge this? :)

@bufdev
Copy link
Contributor Author

bufdev commented Jun 13, 2017

Should be done. I had some issues with ./fixup.sh so I did the merge of util/metautils/DOC.md manually - this might not be the right output.

@NeoCN
Copy link

NeoCN commented Aug 18, 2017

@peter-edge When will this pr be merged? I got compile error when use with latest gRPC code, in this commit grpc/grpc-go@596a6ac, the NewContext and FromContext was removed.

@bufdev
Copy link
Contributor Author

bufdev commented Aug 18, 2017

It's up to @mwitkow, I'm not a maintainer

@mwitkow mwitkow merged commit a610003 into grpc-ecosystem:master Aug 25, 2017
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.

4 participants