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

HomeDBManager refactoration: extract method related to Users management in its own module #1049

Merged
merged 14 commits into from
Jun 18, 2024

Conversation

fflorent
Copy link
Collaborator

@fflorent fflorent commented Jun 14, 2024

Context

The HomeDBManager module is too big, it has too much responsibilities and it becomes hard to have a global vision of its methods.

Proposed solution

I extracted some methods related to users management and moved them in their own class: UsersManager. It's a first step for shrinking HomeDBManager.

The HomeDBManager remains the exposed class to the other parts of the code: any module under gen-server/lib/homedb like UsersManager is intended to be used solely by HomeDBManager, and in order to use their methods, an indirection has to be created to pass through HomeDBManager.

In a later PR, I'll propose to move HomeDBManager.ts to homedb/index.ts. I prefer deferring in order to remove noise and make small steps instead.

How to review?

I don't know whether Github has an option to see line moved across files. I found this trick which uses the git diff command:

$ git checkout refactor-homedb # or gh pr 1049
$ git diff --color-moved=zebra origin/main HEAD

Source: https://git-scm.com/docs/diff-options#Documentation/diff-options.txt---color-movedltmodegt

@fflorent fflorent marked this pull request as ready for review June 14, 2024 15:08
hexaltation

This comment was marked as duplicate.

Copy link
Collaborator

@hexaltation hexaltation left a comment

Choose a reason for hiding this comment

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

LGTM

@fflorent
Copy link
Collaborator Author

Thank you @hexaltation!

@paulfitz paulfitz self-requested a review June 17, 2024 13:57
Copy link
Member

@paulfitz paulfitz left a comment

Choose a reason for hiding this comment

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

Thank you for wrestling with this beast @fflorent

@fflorent
Copy link
Collaborator Author

Thanks @paulfitz! The PR is not merged yet, do you expect something else beforehand?

@paulfitz paulfitz merged commit 95b2459 into gristlabs:main Jun 18, 2024
13 checks passed
@paulfitz
Copy link
Member

Thanks @paulfitz! The PR is not merged yet, do you expect something else beforehand?

Thanks for reminder :)

CamilleLegeron pushed a commit to incubateur-territoires/grist-core that referenced this pull request Jun 20, 2024
…nt in its own module (gristlabs#1049)

The HomeDBManager remains the exposed class to the other parts of the code: any module under gen-server/lib/homedb like UsersManager is intended to be used solely by HomeDBManager, and in order to use their methods, an indirection has to be created to pass through HomeDBManager.
SleepyLeslie pushed a commit that referenced this pull request Jul 8, 2024
…nt in its own module (#1049)

The HomeDBManager remains the exposed class to the other parts of the code: any module under gen-server/lib/homedb like UsersManager is intended to be used solely by HomeDBManager, and in order to use their methods, an indirection has to be created to pass through HomeDBManager.
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.

3 participants