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

go 1.7 context #54

Merged
merged 5 commits into from
Sep 11, 2016
Merged

go 1.7 context #54

merged 5 commits into from
Sep 11, 2016

Conversation

skabbes
Copy link
Collaborator

@skabbes skabbes commented Sep 11, 2016

Refactor value storage to use net.Context. This allows users of bone to use http.Request.WithContext without bone breaking.

I also expect this to have performance benefits since the map of requests has been completely eliminated (and locking is no longer required, since we don't allow mutations of the request values).

This also addresses a bug where using net.Context causes bone to
silently fail due to http.Request.WithContext not preserving pointer
identity.
@skabbes skabbes force-pushed the feature/go_17_context branch 5 times, most recently from a69b9d1 to 4600a3c Compare September 11, 2016 07:27
}

vars := make(map[string]string, totalSize)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Instead of letting the map dynamically re-allocate when we need it to. I am pre-counting the space that will be needed and allocating exactly that amount of space.

@squiidz
Copy link
Member

squiidz commented Sep 11, 2016

Awesome work ! Probably the best PR i've ever get !! For the breaking changes in your code, i don't mind if it's not a major one, maybe just indicate it in the changelog would be enough.

@skabbes
Copy link
Collaborator Author

skabbes commented Sep 11, 2016

Ok, I actually changed it to have no breaking changes. I think as a developer, this is what I would have wanted when upgrading.

Would love to get this merged so I can start using bone + net.Context very soon !!

@squiidz squiidz merged commit 119def6 into go-zoo:master Sep 11, 2016
@skabbes skabbes deleted the feature/go_17_context branch September 11, 2016 17:38
@squiidz
Copy link
Member

squiidz commented Sep 11, 2016

Are you interested to become a bone maintainer ?

@skabbes
Copy link
Collaborator Author

skabbes commented Sep 12, 2016

Hey @squiidz I don't know how much time I can dedicate to it, but I know the code and use cases well enough that I can help out when I can. Do you have a roadmap of things you want to add?

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.

2 participants