Skip to content
This repository has been archived by the owner on Feb 11, 2022. It is now read-only.

Clarify the relation between SQLiteContentProviderImpl and ExtendedSQLiteOpenHelper #67

Closed
wants to merge 2 commits into from

Conversation

friedger
Copy link
Contributor

@friedger friedger commented Dec 1, 2016

This PR clarifies the need of SQLiteContentProviderImpl for an ExtendedSQLiteOpenHelper.

getDatabaseHelper is overwritten and returns the ExtendedSQLiteOpenHelper.

@stefanhoth
Copy link
Contributor

Can one of the admins review this PR?

@friedger friedger changed the title Fixes #57 Clarify the relation between SQLiteContentProviderImpl and ExtendedSQLiteOpenHelper Dec 1, 2016
@frapontillo
Copy link
Contributor

CI, please test this.

Copy link
Contributor

@frapontillo frapontillo left a comment

Choose a reason for hiding this comment

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

So this is basically moving the casting to another method that can be easily overridden.

@friedger
Copy link
Contributor Author

friedger commented Dec 5, 2016

And it forces sub classes to also use the ExtendedSQLiteOpenHelper.

@juankysoriano
Copy link
Contributor

@friedger you mention that this fixes #57, correct me if I am wrong but seems that rather than fixing formalises the expected usage, right? The person opening that issue wanted to use his own SQLiteOpenHelper, your PR instead makes explicit the need for ExtendedSQLiteOpenHelper; am I right?

@blundell
Copy link
Contributor

blundell commented Dec 5, 2016

I agree with @juankysoriano

And it forces sub classes to also use the ExtendedSQLiteOpenHelper.

we don't want to force this do we? Rather we should get rid of the explicit cast and code that class to the interface

@friedger
Copy link
Contributor Author

friedger commented Dec 5, 2016

I have removed the reference to #57 . So, this does not fix it, but helps users to understand the implementation better until #57 is fixed.

Copy link
Contributor

@juankysoriano juankysoriano left a comment

Choose a reason for hiding this comment

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

So, this does not fix it, but helps users to understand the implementation better until #57 is fixed.

Yeah, I think this is better than stating this fixes the issue. And also better that having the current side-effect.

The ideal solution to #57 will then be something along the lines of what @blundell suggests.

Leaving this to @blundell in case he has more comments, thanks for the contribution @friedger

@blundell
Copy link
Contributor

blundell commented Dec 5, 2016

If we merged this solution it would couple us tighter to the current implementation.

Right now it is an implicit cast that the public API doesn't know about. Merging this change would make the contract explicit but it would also mean a non backwards compatible change i.e. compile errors when people upgrade - then if we did the fix for #57 we would change the signatures back and people would again have a break in the API contract.

I don't think we should merge this change.

Sorry 👍

@blundell blundell closed this Dec 5, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants