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

ctx in Session.Set #87

Merged
merged 2 commits into from
Apr 19, 2020
Merged

ctx in Session.Set #87

merged 2 commits into from
Apr 19, 2020

Conversation

bheni
Copy link
Contributor

@bheni bheni commented Apr 17, 2020

No description provided.

Brian Hendriks added 2 commits April 16, 2020 19:20
Signed-off-by: Brian Hendriks <brian@liquidata.co>
Signed-off-by: Brian Hendriks <brian@liquidata.co>
Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

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

LG except for mixing context interfaces

@@ -88,10 +88,11 @@ func (s *BaseSession) Address() string { return s.addr }
func (s *BaseSession) Client() Client { return s.client }

// Set implements the Session interface.
func (s *BaseSession) Set(key string, typ Type, value interface{}) {
func (s *BaseSession) Set(ctx context.Context, key string, typ Type, value interface{}) error {
Copy link
Member

Choose a reason for hiding this comment

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

Should be a *sql.Context

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would create a weird circular reference that I don't really like.

@zachmu
Copy link
Member

zachmu commented Apr 19, 2020

I'm merging this because you already merged changes in dolt that rely on it

@zachmu zachmu merged commit 4af6ede into ld-master Apr 19, 2020
@Hydrocharged Hydrocharged deleted the bh/ctx-in-session-set branch April 23, 2020 18:39
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

2 participants