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

Define abstractions #10

Merged
merged 1 commit into from
May 15, 2023
Merged

Define abstractions #10

merged 1 commit into from
May 15, 2023

Conversation

HectorNarvaez
Copy link
Contributor

Define the abstractions that will be exposed to the users

This PR is related to #7, in order to split it into multiple smaller PR's

This PR depends on #9 and can see easy diff clicking here.

Only merge this once #9 is merged.

@HectorNarvaez HectorNarvaez added Don't merge Can't be merged yet Depends on This have dependency with another PR labels May 15, 2023
@HectorNarvaez HectorNarvaez mentioned this pull request May 15, 2023
interface OmhStorageClient {

interface Builder {
fun build(context: Context): OmhStorageClient
Copy link
Contributor

Choose a reason for hiding this comment

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

You should also request the OmhAuthClient. Let me know when you're available to help you setup that dependency.

fun build(context: Context): OmhStorageClient
}

fun setupAccessToken(token: String)
Copy link
Contributor

Choose a reason for hiding this comment

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

You should depend on the OMH Auth library to handle the access token and it's refreshing

import android.content.Context
import kotlin.reflect.KClass

object OmhStorageProvider {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a TODO comment to indicate that this needs to be refactored in the future once there's more code done for GMS and non GMS

return outputList
}
for (gDriveFileOrFolder in inputList) {
outputList.add(
Copy link
Contributor

Choose a reason for hiding this comment

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

could you extract the content of the for loop into a separate function to reduce indentation?

@@ -1 +1,14 @@
package com.omh.android.storage.api.data.model

interface RemoteFileOrFolder
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we benefit of using a sealed class here instead?

import com.omh.android.storage.api.domain.model.FileOrFolder
import com.omh.android.storage.api.domain.model.Folder

fun fromFilesFoldersRemoteListToDomain(inputList: List<RemoteFileOrFolder>): List<FileOrFolder> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be feasible using a .map() function and passing the mapping function.

@@ -1 +1,17 @@
package com.omh.android.storage.api.domain.model

interface FileOrFolder {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here with the sealed class

val modificationDate: Long
}

data class File(
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you using the additional functions of the data class implementation? If not, I'd suggest removing the anotation

interface GetAllFilesAndFolders {
fun execute()
fun execute(): List<FileOrFolder>
Copy link
Contributor

Choose a reason for hiding this comment

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

You should return an OmhTask object. You can copy the implementation from the auth lib.

@HectorNarvaez HectorNarvaez removed Don't merge Can't be merged yet Depends on This have dependency with another PR labels May 15, 2023
@HectorNarvaez
Copy link
Contributor Author

HectorNarvaez commented May 15, 2023

@Anwera64 as the architecture will change, i think the comments will be solved in future PR's

This will be solved in #8

Copy link
Contributor

@Anwera64 Anwera64 left a comment

Choose a reason for hiding this comment

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

Looking forward for the changes in #8

@HectorNarvaez HectorNarvaez merged commit 45b9631 into main May 15, 2023
2 checks passed
@HectorNarvaez HectorNarvaez deleted the state/defineAbstractions branch May 15, 2023 22: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.

None yet

2 participants