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

02-room #6

Closed
wants to merge 2 commits into from
Closed

02-room #6

wants to merge 2 commits into from

Conversation

Serchinastico
Copy link
Contributor

This PR is not meant to be merged

This is how to include Room inside the project:

  • Include Room dependencies. We are including coroutines support to use suspend functions in the Dao.
  • Create Database,Dao and Entity to store SuperHeroes. We are using all four CRUD methods.
  • Include a migration, you can see the previous model before where we were storing the very same fields we were using for a SuperHero. Now we use the @Embedded annotation to get an object and split all its fields into fields of the table (preffixed with the string "superhero_").
  • Replace LocalSuperHeroDataSource implementation by the new Room database.

Copy link
Member

@davideme davideme left a comment

Choose a reason for hiding this comment

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

I know it's not part of this PR, but for the slides, I'm not sure of the added value of testing tables, it's described in the doc. I wouldn't include it here.

@Entity(tableName = "superheroes")
data class SuperHeroEntity(
@PrimaryKey val id: String,
@Embedded(prefix = "superhero_") val superHero: SuperHero
Copy link
Member

Choose a reason for hiding this comment

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

Seems to me that Embedded is an advanced feature of Room, we could simplify this for the course, even if it's more verbose.

Copy link
Member

Choose a reason for hiding this comment

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

Also not sure about the effect you want to have as SuperHero as already an id and the prefix of these fields.

the table will have
superheroes.id and superheroes. superhero_id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it was just a test to see how @Embedded work, in this case we had two copies of the same model, SuperHero and SuperHeroEntity, just wanted to try having the domain model stored in DB instead of a DB-only model.

If we go this way we can remove the outer id and configure the primary key from the @Entity annotation instead.

import com.karumi.jetpack.superheroes.data.repository.room.SuperHeroDao
import com.karumi.jetpack.superheroes.data.repository.room.SuperHeroEntity

@Database(entities = [SuperHeroEntity::class], version = 2)
Copy link
Member

Choose a reason for hiding this comment

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

There is a neat way to test database migration would be nice to have it included, even if not required during the exercises.

https://developer.android.com/training/data-storage/room/migrating-db-versions#export-schema

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the PR is mid development, yesterday I started creating all tests and I'm using that approach, I only exported v2 schema because I did it late in the process but I'm adding schema-v1 as well

@davideme
Copy link
Member

Why not split this exercise into two parts? First without migration for devs, that just want to download everything again using .fallbackToDestructiveMigration() and a second part with migration and advanced notation of tables and testing of the migrations?


private val from1To2 = object : Migration(1, 2) {
override fun migrate(database: SupportSQLiteDatabase) {
database.execSQL("DROP TABLE superheroes")
Copy link
Member

Choose a reason for hiding this comment

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

Was there a superheroes table before ?

@davideme
Copy link
Member

Just realize that version 1 of the database is in the first commit, but not visible in the PR.
For this additional reason, I would split this into 2 branches.

@Serchinastico
Copy link
Contributor Author

I'm going to split the exercise in 2 as you said, 02a-room having the basics of Room with no migrations and 02b-room-migrations with the migration to use @Embedded, migrations and migration tests. I'll let you know when I'm done

@davideme
Copy link
Member

davideme commented Jan 30, 2019 via email

@Serchinastico
Copy link
Contributor Author

I'm closing this PR in favor of #7 and #8 . The first PR just includes Room and the second one deals with migrations and testing them, sort of a basic vs advanced usage of the lib. Thanks for the comments @davideme 👍

@Serchinastico Serchinastico deleted the 02-room branch January 30, 2019 10:47
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.

2 participants