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

Methods/properties missing from IDatabase #500

Open
asherber opened this issue Jan 2, 2019 · 5 comments
Open

Methods/properties missing from IDatabase #500

asherber opened this issue Jan 2, 2019 · 5 comments

Comments

@asherber
Copy link
Collaborator

asherber commented Jan 2, 2019

There appear to be some public methods and properties of Database which are missing from IDatabase (and from any of the other interfaces which IDatabase implements). I wasn't sure if this was intentional or not; I ran into it because IDatabaseBuildConfiguration.Create() returns an IDatabase, and I noticed that there were some things I couldn't do.

// These should be accessible from the interface
Connection property
KeepConnectionAlive property
CreateCommand();

// These two should probably be marked protected instead of public.
// They're part of the plumbing -- are there times when a user should call them directly?
OpenSharedConnection()
CloseSharedConnection()

// This one seems like it might be protected or private instead of public,
// otherwise add to interface.
FormatCommand()

// These should probably be protected, since they're only used by Database 
// descendants, not called directly.
OnBeginTransaction() and friends
@pleb
Copy link
Member

pleb commented Apr 11, 2019

Good eye. We should fix this.

@asherber
Copy link
Collaborator Author

I'm happy to put together a PR if you let me know which way you'd like to go. Note that moving some public methods to protected or private, as I suggest, is a breaking change, even if it might be better design. For that matter, adding methods to an interface is also a breaking change for anyone who has made their own object to implement IDatabase.

@pleb
Copy link
Member

pleb commented Apr 14, 2019

Yeah, true. Ok, then we should bump the version to 6.1

@asherber
Copy link
Collaborator Author

(Under sematic versioning, probably 6.1 should have been when stored procs were added, and 6.2 should be async. A breaking change would mean 7.0.)

@pleb
Copy link
Member

pleb commented Apr 25, 2019

Happy to roll version 7

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

No branches or pull requests

2 participants