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

Change transactionKey to be const int 0 #38

Closed
wants to merge 1 commit into from

Conversation

derekperkins
Copy link
Contributor

Based on examples from https://blog.golang.org/context, I changed transactionKey from being a string with a pointer key to a simple const = 0.

Based on examples from https://blog.golang.org/context, I changed `transactionKey` from being a string with a pointer key to a simple const = 0.
@derekperkins
Copy link
Contributor Author

I made this PR from the web interface, so sorry if I broke something unintentionally, I wasn't able to build or test the code. 😄

@derekperkins
Copy link
Contributor Author

#34

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 0879471 on derekperkins:patch-2 into 7108350 on qedus:managedvm.

@derekperkins
Copy link
Contributor Author

@jongillham I'm not super passionate about this PR. I still haven't transitioned my code to using context.Context, so this is my first introduction too. I thought about asking on the other thread about about making it a const, but figured it would be faster to just submit a PR to start the discussion.

Not making it it's own type was just an oversight; the docs are very clear on that point. I'm not against using a string pointer if you feel like that is where more current Google projects are moving. I was just approaching it from the standpoint of less memory allocation, but it doesn't matter either way. I would be interested in looking at any repos you remember seeing that design pattern in.

You may have seen my question that I just asked in the Google Group about embedded structs. It sounds like our design pattern is the same, as I simply embedded the appengine.Context struct in my own Context and shipped that around everywhere. This new design pattern is weird for me to grok, so I'm still trying to wrap my head around how I want to rewrite my code. I don't particularly want to type assert every time that I access my user or other context scoped info and rewrite all my code to use it that way, but that also makes me feel like I'm missing something.

@jongillham
Copy link
Member

The main code I saw using context.Context with a string key was the google.golang.org/appengine repository for example: https://github.com/golang/appengine/blob/master/internal/transaction.go#L35. However I think I prefer the efficiency of your pull request now.

Hah, I just saw your google-appengine-go post. This is exactly my issue as well and originally why I wanted to completely change the API so transactions worked properly with this pattern because as soon as you embeded a transactional appengine.Context into something else there was no way for the nds.Get/Put/Delete methods to know it needs to deal with the call as a transaction.

What I have spent the last two days doing to convert my code is exactly what the context.Context docs suggest and create separate FromContext and NewContextfunctions within my packages.

    func myFunc(c context.Context) error {
        myValue, ok := mypackage.FromContext(c)
        if !ok {
            return errors.New("myValue required in context")
        }
    }

So far it seems like a reasonable approach and I have not run into any issues.

I'll close this pull request and accept one with a typed integer constant instead if you feel that way inclined.

@jongillham jongillham closed this Apr 7, 2015
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.

None yet

3 participants