Open ILIYANGERMANOV opened 7 months ago
Hey @Rick-AB wanna try this one? It's kinda challenging but you already did a great job with the data layer 💯
Thank you @ILIYANGERMANOV for raising Issue #2961! 🚀 What's next? Read our Contribution Guidelines 📚.
Tagging @ILIYANGERMANOV for review & approval 👀
Hey @Rick-AB wanna try this one? It's kinda challenging but you already did a great job with the data layer 💯
sure, let's give it a bash!
I'm on it
Thank you for your interest @Rick-AB! 🎉 Issue #2961 is assigned to you. You can work on it! ✅
If you don't want to work on it now, please un-assign yourself so other contributors can take it.
Also, make sure to read our Contribution Guidelines.
@ILIYANGERMANOV migrating to the repository would also mean the ui layer will now use the new account model, correct?
@ILIYANGERMANOV migrating to the repository would also mean the ui layer will now use the new account model, correct?
@Rick-AB yup correct 💯
@ILIYANGERMANOV I discovered this class in the legacy module is used in the ui layer and uses the deprecated account model. to me it doesn't make sense to use the new account model here instead since we're carrying out a migration. let me know what you think
@ILIYANGERMANOV I discovered this class in the legacy module is used in the ui layer and uses the deprecated account model. to me it doesn't make sense to use the new account model here instead since we're carrying out a migration. let me know what you think
Yes, it makes sense. Ideally in the UI layer we should have a different AccountViewState
model that has only the things that are to be displayed and must consist only of primitives and @Immutable
so Compose can be smooth. See #2965
Yes, it makes sense. Ideally in the UI layer we should have a different
AccountViewState
model that has only the things that are to be displayed and must consist only of primitives and@Immutable
so Compose can be smooth. See #2965
if I understand you correctly, you're saying the legacy Account
model in the AccountData
class CAN be replaced with the new model???
We should have:
See our Architecture guidelines. But that's a bigger change so feel free to implement it as you see fit
@ILIYANGERMANOV I've migrated the accounts tab, but I want you to take a look so we can decide if we should proceed with it or scrape it : )
there are some "not so elegant solutions" trying to patch some classes like CalcWalletBalanceAct
, CalcAccIncomeExpenseAct
etc pending when other migrations become active.
should I create a draft pr?
@Rick-AB yeah, please create a draft PR and I'll have a look when I have time
@ILIYANGERMANOV can we reopen this issue to continue the migration or will a new issue be created?
@ILIYANGERMANOV I noticed the there is no flagDeleted
in AccountRepository, did we miss that out or is there a reason?
@ILIYANGERMANOV I noticed the there is no
flagDeleted
in AccountRepository, did we miss that out or is there a reason?
We no longer need that operation but feel free to add it for backwards compatibility
@ILIYANGERMANOV while migrating TransactionsScreen to use new Account model, I noticed some common composables used across many screen take in the legacy model in some way so I'm unable to migrate this screen without affecting the others. we need some kind of workaround in other not to change all these files at once, I'm thinking a temporary extension mapper that maps between these two models?
what's your opinion on this?
@ILIYANGERMANOV while migrating TransactionsScreen to use new Account model, I noticed some common composables used across many screen take in the legacy model in some way so I'm unable to migrate this screen without affecting the others. we need some kind of workaround in other not to change all these files at once, I'm thinking a temporary extension mapper that maps between these two models?
what's your opinion on this?
@Rick-AB Ideally, the UI layer should not depend on the domain models. We should create AccountViewState
(only with primitives in :shared:ui) and recreate all common composables. That's the right way.
For a workaround, I have no good ideas atm 😄 If you can figure out sth that won't butcher the performance it would be cool!
@ILIYANGERMANOV I'm unassigning myself from this issue as I currently don't have the time bandwidth to continue working on this for now
Please confirm the following
What would you like to improve?
AccountRepository
to read/write dataAccountDao
andWriteAccountDao
usages with the repositoryBecause...
Description
No response
Success Criteria