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

Added Close method to QueryExecutor interface and Mock implementation #427

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

krystsinaetc
Copy link

@krystsinaetc krystsinaetc commented Sep 18, 2017

I added Close() method to Mock, because without it it's impossible to use Mock as QueryExecutor interface.

@CMogilko
Copy link
Member

This PR is not only adding method to the Mock. It's about adding method Close() to the QueryExecutor interface. That API change may break people's self-written mocks.

@krystsinaetc
Copy link
Author

krystsinaetc commented Sep 18, 2017

@CMogilko Yes, and this is the problem! It would be great to close session somehow:

type RethinkStore struct {
	session r.QueryExecutor // can't call Close() for *Session
	conf    *RethinkConf
}

@krystsinaetc krystsinaetc changed the title Added Close method to Mock Added Close method to QueryExecutor interface and Mock implementation Sep 18, 2017
@krystsinaetc
Copy link
Author

krystsinaetc commented Sep 18, 2017

@CMogilko other solution is to create a separate interface which the same as current interface but additionally includes Close method, could it be worth? Otherwise, I will have to create a new interface in my own program as well as Mock structure with gorethink embedded Mock structure and this Close method.

@CMogilko
Copy link
Member

This PR is useful for testing, but it changes the API, so it can be accepted in v4.0. And it is need more "smart" mocking of Close().

@gabor-boros
Copy link
Member

@CMogilko what did you mean "more smart" mocking exactly?

@CMogilko
Copy link
Member

I mean the ability to set error from tests, not only return nil

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants