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

Major modifications: Addition of ContentProvider and column name changed from id to _id. #556

Closed
wants to merge 20 commits into from

Conversation

bpappin
Copy link

@bpappin bpappin commented Mar 30, 2016

These are some major modifications in order to support several new features, most important among them, a new ContentProvider, so you can work with CursorLoader and the CursorAdapters.

This code is not compatible with version 1.5 of Sugar, mainly because the id column has been changed to _id as defined in android.provider.BaseColumns._ID (should have been like that from the start IMO) and I don't expect it to be merged.

At this point I am just making sure I share the code back to the project.

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @whoshuu, @sibeliusseraphini and @pguedes to be potential reviewers

@sibelius
Copy link
Contributor

I will take a look tomorrow, thanks

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.04%) to 65.448% when pulling c7c8e71 on bpappin:master into 619b5db on satyan:master.

@pguedes
Copy link
Contributor

pguedes commented Mar 30, 2016

Ok. Will try to have a look when i can...

Enviado do meu iPad

No dia 30/03/2016, às 22:13, Mention Bot notifications@github.com escreveu:

By analyzing the blame information on this pull request, we identified @whoshuu, @sibeliusseraphini and @pguedes to be potential reviewers


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

@@ -1,6 +1,14 @@
apply plugin: 'com.android.library'
apply from: '../maven_push.gradle'

// https://docs.gradle.org/current/userguide/publishing_maven.html
// http://www.flexlabs.org/2013/06/using-local-aar-android-library-packages-in-gradle-builds
apply plugin: 'maven-publish'
Copy link
Contributor

Choose a reason for hiding this comment

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

@bpappin using maven-publish plugin, we can remove maven_push.gradle right?

cc @satyan

@sibelius
Copy link
Contributor

@bpappin it looks like you are using tabs instead of space, could you fix this?

You did a great job on this PR thanks, I will make a few comments on your code, so we can accept this

We can add this to Sugar 2.0 release

Let me know if you want to help on more features like these ones (#540)

@sibelius
Copy link
Contributor

could you also put in the Sugar example how to use the SugarContentProvider?

And maybe add some docs?

If you have any questions you could chat with me here: https://gitter.im/satyan/sugar or https://gitter.im/sibeliusseraphini or twitter

@bpappin
Copy link
Author

bpappin commented Apr 1, 2016

I'm not sure how to fix the tabs, I always use tabs, so my editors are configured for them.

@sibelius
Copy link
Contributor

sibelius commented Apr 1, 2016

I'm sure that every editor has the option to change tabs for spaces, don't you use android studio?

@bpappin
Copy link
Author

bpappin commented Apr 1, 2016

I do, i just have no idea how to switch it, as it's never been an issue with modern editors, compilers or languages :)

Which class specifically needs to be fixed? I was trying not to get the editor to change them, so hopefully it's not every file.

@bpappin
Copy link
Author

bpappin commented Apr 1, 2016

By the way, feel free to edit as you see fit, I will not be offended.
There are other things I want to do to it, so I'm going to remain on my own fork for a while.

@bpappin bpappin changed the title Major modifications Major modifications: Addition of ContentProvider and column name changed from id to _id. Apr 1, 2016
# Conflicts:
#	library/src/main/java/com/orm/SchemaGenerator.java
#	library/src/main/java/com/orm/SugarContext.java
#	library/src/test/java/com/orm/SchemaGeneratorTest.java
#	sugar.iml
@bpappin
Copy link
Author

bpappin commented Apr 2, 2016

I've just merged your master into my master, so the pull request should be in sync again.

@sibelius
Copy link
Contributor

sibelius commented Apr 2, 2016

@bpappin do you have an idea or some code of how to migrate a production app using id with Sugar 1.5 to Sugar 2.0 using _id?

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.2%) to 68.185% when pulling 1cf0280 on bpappin:master into 5007b36 on satyan:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.0%) to 68.441% when pulling b294404 on bpappin:master into 5007b36 on satyan:master.

@sibelius
Copy link
Contributor

sibelius commented Apr 3, 2016

Do you think that a migration script could fix this?

@bpappin
Copy link
Author

bpappin commented Apr 3, 2016

I don't know, someone is going to have to test it.
I suspect it could, but I've never tried to alter an id column in an sqlite table.

@JonatanSalas
Copy link
Collaborator

Could you solve the conflicts?

@bpappin
Copy link
Author

bpappin commented Apr 11, 2016

Yes, need to do a sync with the main branch anyway.

I had to do some messing around with configuration because the class scanner stopped working when I updated to Android Studio 2.0.0 and updated Gradle.
I am not positive what caused that issue, but it was something to do with the Dex loader.

@sibelius
Copy link
Contributor

we want to release this as an alpha version of Sugar 2.0

@bpappin
Copy link
Author

bpappin commented Apr 11, 2016

There are a lot of pull results that will need working through, I will try and get them merged shortly.

I suspect this code will need some cleanup and testing done as right not it works for me, but I haven't tried anywhere else.

@JonatanSalas
Copy link
Collaborator

@bpappin There is no problem, we can do the clean up and test! Don't worry about that!

 Please enter the commit message for your changes. Lines starting
@bpappin
Copy link
Author

bpappin commented Apr 12, 2016

I'm having a lot of trouble with the merge, there are a lot of small things that change entire files.
I'm going to have to go through each change and try and match them.

@bpappin
Copy link
Author

bpappin commented Apr 16, 2016

At this stage, I'm wondering if it would be easier to start with a clean head version and redo the changes.
It would take a while, but maybe less time than trying to merge.

@sibelius
Copy link
Contributor

you could create a branch from master, then you could cherry-pick yours commits

@bpappin
Copy link
Author

bpappin commented Apr 19, 2016

I'm just not finding the merge easy, to many little changes, and this week got busy.
It's going to take me hours that I just don't have time for at the moment.

I'll see if I can make some time for it, in a few days.

@shobhik
Copy link

shobhik commented May 29, 2016

Hi, not to sound ungrateful/whiny but when can we expect this to be merged successfully, or see a 2.0 release? I ask because I haven't seen any activity on this thread for a while so I have no idea what timeframe is involved.

We have ~4 apps using Sugar that will enter production in the near future. Data loss being a big deal in our industry, it would make all our lives easier if we didn't have to force our clients to reinstall between 1.x and 2.0 so I'd rather wait to release our apps with Sugar 2.0.

@bpappin @JonatanSalas I'm not the greatest at unit testing but am willing to do physical QA (time permitting) if that will expedite things.

Goes without saying I love this library and the hard work y'all have put into it!

@sibelius
Copy link
Contributor

@shobhik you should talk to @satyan, he is the owner of this lib, he has the keys to deploy a new release.

@bpappin I think it is easy to close this PR and send a new one based on master version

maybe @shobhik could help u with this

@bpappin
Copy link
Author

bpappin commented Jul 31, 2016

OK, so I finally have a few cycles to come back to this.
I'm considering simply forking a new branch and starting over, as there have been too many small changes to make it easy.

What's the status of this pull request from a project perspective at this point?

@bpappin
Copy link
Author

bpappin commented Jun 2, 2023

I am assuming i can close this PR?

@bpappin
Copy link
Author

bpappin commented Jun 2, 2023

Close as obsolete

@bpappin bpappin closed this Jun 2, 2023
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.

7 participants