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

Make caller serialization configurable #327

Merged
merged 5 commits into from
Mar 14, 2017

Conversation

skipor
Copy link
Contributor

@skipor skipor commented Feb 19, 2017

$GOPATH trim logic based on go-stack/stack which is under Apache license, so can't be put in repo. So required logic have been moved to another repo (skipor/goenv), and exported as third party.
That decision is doubtful, so fell free to propose something better.
WARN: skipor/goenv is not pinned in glide.lock. I don't know how to do this without updating and keeping glide.lock hash valid.

@mention-bot
Copy link

@skipor, thanks for your PR! By analyzing the history of the files in this pull request, we identified @akshayjshah, @peter-edge and @imkira to be potential reviewers.

@bufdev
Copy link
Contributor

bufdev commented Feb 20, 2017

I don't think copying someone else's code out to your own repo is a nice thing to do, also you just copied the license so this doesn't change anything. We were thinking of using Chris Hines' code in #311. This code somewhat duplicates that effort as well.

@skipor
Copy link
Contributor Author

skipor commented Feb 20, 2017

Well, I remove GoPathCallerEncoder commit with external dependency.
This PR still fixes #319, and nice start point for #304.

@flyingmutant
Copy link

Does this PR have a chance to land before 1.0?

@skipor
Copy link
Contributor Author

skipor commented Mar 13, 2017

@peter-edge, @akshayjshah need I fix something, or your will handle this PR yourself?

@akshayjshah
Copy link
Contributor

There's certainly a chance that this can land before 1.0; it's not a breaking change, so we can also land it after 1.0 if necessary.

@skipor we can take it from here - I just need to spend the time to review it carefully :)

Copy link
Contributor

@akshayjshah akshayjshah left a comment

Choose a reason for hiding this comment

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

LGTM, just going to push some small cleanups.

skipor and others added 5 commits March 14, 2017 10:16
@akshayjshah akshayjshah changed the title CallerEncoder and trim caller file to $GOPATH Make caller encoding configurable Mar 14, 2017
@akshayjshah akshayjshah changed the title Make caller encoding configurable Make caller serialization configurable Mar 14, 2017
@akshayjshah akshayjshah merged commit 640110d into uber-go:master Mar 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants