-
Notifications
You must be signed in to change notification settings - Fork 608
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
refactor(core): migrate from dbflow to room for client #2300
base: kmp-impl
Are you sure you want to change the base?
Conversation
@@ -17,6 +17,6 @@ import kotlinx.parcelize.Parcelize | |||
*/ | |||
@Parcelize | |||
class IdentifierTemplate( | |||
var allowedDocumentTypes: List<DocumentType>? = ArrayList(), | |||
val allowedDocumentTypes: List<DocumentType>? = ArrayList(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use emptyList() (or mutableListOf() if mutability is necessary) in instead of ArrayList(). It is more efficient to use emptyList() since it prevents needless object construction and offers superior immutability by default. For improved efficiency and clarity, use emptySet(), emptyMap(), listOf(), setOf(), and mapOf() when appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will do as suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested changes implemented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's few things to change, these changes are not nessacary as per your task, but as you modifying repository classes this changes should required
-
Non-suspend functions returning
Flow
:
If a function returns aFlow
, it should not be marked assuspend
becauseFlow
is already designed to handle asynchronous streams.fun getItems(): Flow<List<Item>> { // Return a Flow of List<Item> }
-
Convert
List<T>
toFlow<List<T>>
:
If a function currently returns aList<T>
, it should be modified to return aFlow<List<T>>
to align with reactive programming practices.fun getItems(): Flow<List<Item>> { // Convert List<Item> to Flow<List<Item>> }
-
Suspend functions for actions (create/update/delete):
If a function performs an action like creating, updating, or deleting, it should be marked assuspend
and return aResult
orUnit
.suspend fun createItem(item: Item): Result<Unit> { // Perform create action and return Result } suspend fun updateItem(item: Item): Result<Unit> { // Perform update action and return Result } suspend fun deleteItem(itemId: String): Result<Unit> { // Perform delete action and return Result }
@niyajali Thanks for reviewing! I will do as suggested. |
de52712
to
1fedc46
Compare
fun readAllClients(): Flow<Page<Client>> | ||
|
||
@Query("SELECT * FROM GroupTable WHERE id = :groupId") | ||
fun getGroupAssociateClients(groupId: Int): Flow<GroupWithAssociations> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove Flow from here. Don't use Flow in dao unless we are retrieving list. Check the entire DAO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@biplab1 my apologies. Ignore the previous comment.
Check if we need a single item. If yes add 'LIMIT 1`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In DatabaseHelperClient.kt, this function was returning Observable<GroupWithAssociations>
, so it was changed to Flow<GroupWithAssociations>
fun getClient(clientId: Int): Flow<Client> | ||
|
||
@Insert(onConflict = OnConflictStrategy.REPLACE) | ||
fun saveClientAccounts( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Save, delete and update should be a suspend function always
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does your statement hold true even when the function returns a flow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes,
save, delete and update should not return flow because they are one time operations
|
||
var additionalProperties: Map<String, Any> = HashMap(), | ||
val additionalProperties: Map<String, Any> = HashMap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
emptyMap() would be better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be implemented.
@@ -186,7 +186,7 @@ class DataManagerClient @Inject constructor( | |||
* @param clientId Client ID | |||
* @return ResponseBody is the Retrofit 2 response | |||
*/ | |||
fun deleteClientImage(clientId: Int): Observable<ResponseBody> { | |||
fun deleteClientImage(clientId: Int): Flow<ResponseBody> { | |||
return mBaseApiManager.clientsApi.deleteClientImage(clientId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mBaseApiManager is used for the api call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand your statement, could you please provide more details?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for the api call. But we are migrating the database only right?
But if you want to migrate it, we need to change it to something like that.
@DELETE(APIEndPoint.CLIENTS + "/{clientId}/images")
suspend fun deleteClientImage(@Path("clientId") clientId: Int): ResponseBody
fun deleteClientImage(clientId: Int): Flow<ResponseBody> {
return flow { mBaseApiManager.clientsApi.deleteClientImage(clientId) }
}
@itsPronay Thanks for reviewing! I will look into your suggestions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicate [ignore this comment]
Fixes - Jira-#341
Didn't create a Jira ticket, click here to create new.
Please Add Screenshots If there are any UI changes.
Please make sure these boxes are checked before submitting your pull request - thanks!
Run the static analysis check
./gradlew check
orci-prepush.sh
to make sure you didn't break anythingIf you have multiple commits please combine them into one commit by squashing them.