Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants