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

Refactor Manager #3062

Merged
merged 19 commits into from
Jun 27, 2024
Merged

Refactor Manager #3062

merged 19 commits into from
Jun 27, 2024

Conversation

dickermoshe
Copy link
Collaborator

@dickermoshe dickermoshe commented Jun 26, 2024

This PR does the following:

  • Cherry Pick the Flutter CI fixes from the drift_flutter branch
  • Add a option for combining successive filter calls with an OR instead of a hardcoded AND.
  • Rename Generics in the Manager API to more descriptive names ($Table instead of T)
  • Make Manager classes non-const for now
  • Rename some fields on ManagerState
  • Remove unnecessary code from Manager API (_getChildManagerBuilder)
  • Add distinct option for some queries

I will create the next PR once this is merged.

NOTE: I "fixed" a "bug" and then reverted it.
Lmk if I should go back and undo the commits so we should have a cleaner git history.

I want to make this as painless for you as possible.
The next feature (references) will introduce another level of complexity and I don't want to be the only person with a strong understanding of it.

I'll be documenting the next PR very clearly.
I'm looking for constructive criticism if you ever have any for me.

@dickermoshe
Copy link
Collaborator Author

@simolus3
This PR is done.
We are ready for review

Copy link
Owner

@simolus3 simolus3 left a comment

Choose a reason for hiding this comment

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

Thanks! I agree that the full names for the type arguments make the manager code much easier to grasp (and the new companion typedef names are also clearer). I don't think we need to rebase/clean up the history.

Revisiting the code, I'm not quite sure what justifies having ProcessedTableManager as a separate class right now, is this required for references or another feature you have in mind that would be breaking without it?

drift/lib/src/runtime/manager/filter.dart Outdated Show resolved Hide resolved
drift/lib/src/runtime/manager/manager.dart Show resolved Hide resolved
drift/lib/src/runtime/manager/manager.dart Outdated Show resolved Hide resolved
drift/lib/src/runtime/manager/manager.dart Show resolved Hide resolved
drift/lib/src/runtime/manager/manager.dart Outdated Show resolved Hide resolved
@dickermoshe
Copy link
Collaborator Author

@simolus3
Made changes and ready for supplemental review

@simolus3
Copy link
Owner

Thanks for the quick follow-up and the detailed explanation! I'll check them tomorrow, I wanted to get the docs update for drift_flutter done today and that took more time than expected.

Copy link
Owner

@simolus3 simolus3 left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to me now!

drift/lib/src/runtime/manager/manager.dart Show resolved Hide resolved
@simolus3 simolus3 merged commit 06dbcc7 into simolus3:develop Jun 27, 2024
11 checks passed
@dickermoshe
Copy link
Collaborator Author

@simolus3

We have 2 ways to proceed for a UserWithReferences class.

Option 1: Wrap the Class

// Implementation
class UserWithReferences {
  UserWithReferences(this.user)
  final User user;
  final GroupManager get group => GroupManager(...).filter(...);
}

// Usage

final usersWithRefs = await users.withReferences(...).get()
for (var userWithRefs in usersWithRefs){
  final user = userWithRefs.user;
  final group = await userWithRefs.group.getSingle()
}

Pros

  • The UserWithReferences class is small
Cons
  • Usage of this class is bloated

Option 2: Extend the Class

// Implementation
class UserWithReferences extends User{
  UserWithReferences(
    super.name,
    super.age,
    super.gender,
   /// ... and all the other fields
)
  final GroupManager get groupManager => GroupManager(...).filter(...);
}

// Usage
final users = await users.withReferences(...).get()
for (var user in users ){
  //  final user = userWithRefs.user; NOT NEEDED
  final group = await user.groupManager.getSingle()
}
Pros
  • Using the results is much cleaner.
  • If one migrated to using a reference at any point, they would not need to refactor.
Cons
  • The generated code would be much longer
  • We couldn't use the term group, because it would clash with the group id field on the original model. We will need to use groupManager instead. However, this isn't so bad. Because a field named group that doesnt return an instance of Group, but rather a GroupManager, doesn't seem that great either.

What are your thoughts?
How could this work for custom classes?

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