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: remove implicit use of db and db.session in model, controllers, etc #503

Closed
23 tasks done
jzohrab opened this issue Oct 30, 2024 · 2 comments
Closed
23 tasks done
Assignees
Labels
code-improvement Make code/structure better good first issue Good for newcomers help wanted Extra attention is needed

Comments

@jzohrab
Copy link
Collaborator

jzohrab commented Oct 30, 2024

BIG NOTE: This work is in progress in branch issue_503_db_session_refactoring

This is a general issue to group several independent issues with the code. In summary, there are many places where there is an implicit dependency on the db.session which Flask manages, which therefore couples the code to Flask unnecessarily.

For example, the various services have from lute import db, and then service methods use db.session.query. It feels to me that instead the service should be classes instantiated with a session object, and use self.session in each method as needed. Then the flask controllers should instantiate the services with the db.session, and use them as before.

Notes on the work:

  • I think that each of these items should be handled separately.
  • If anyone wants to try working on any of them, I'll create a separate issue and assign it to you (or me), so that we can stay reasonably organized.
  • Each change to any object API will impact unit tests as well.
  • I think that these should be tackled in the order presented (approximately), as changes to lower-level items can generally be done gradually before the higher-level items are changed

I'll add to the list of items below as I come across places to work on.

business repos

  • lute/term/model.py, Repository : change the constructor to take a session, and use self.session instead of self.db.session
  • lute/book/model.py, Repository : change the constructor to take a session, and use self.session instead of self.db.session

ORM-level repositories in lute/models/

Many classes have static methods (e.g. find etc) that use a db.session. I'd like to take all of those and move them to Repository classes, e.g, BookTag will have BookTagRepository which will take a session arg in its constructor, and then all static methods can be moved out of BookTag and into the new repo class as instance methods.

  • book.py BookTagRepository
  • book.py BookRepository
  • book.py TextRepository
  • language.py LanguageRepository
  • term.py TermTagRepository
  • term.py StatusRepository
  • term.py TermRepository
  • setting.py ... hmmm not sure about this one, separate repos for each one?

Change parser settings reads

  • currently the Japanese parser reads from the db, but it uses the db.session ... I feel that the settings should be passed in perhaps as a setter method. Different parsers might want different settings ... or perhaps the settings should be stored in environment variables when the settings are set (or loaded)

Convert services to classes (?) or pass session explicitly

Change "service" modules to objects that take a db.session as a parameter, and use that self.session everywhere instead of db.session ?? Optionally, pass the session as a parameter to each function that needs it.

  • lute/termimport/service.py
  • lute/term_parent_map/service.py
  • lute/read/service.py
  • lute/read/render/service.py
  • lute/language/service.py
  • lute/book/service.py
  • lute/backup/service.py
  • lute/themes/service.py
  • lute/stats/service.py

remove db imports where possible

  • lute/db/data_cleanup.py:from lute.db import db
  • lute/db/management.py:from lute.db import db
  • lute/db/demo.py:from lute.db import db
@jzohrab jzohrab added good first issue Good for newcomers code-improvement Make code/structure better labels Oct 30, 2024
@jzohrab jzohrab added this to Lute-v3 Oct 30, 2024
@jzohrab jzohrab added the help wanted Extra attention is needed label Oct 30, 2024
@jzohrab jzohrab self-assigned this Nov 1, 2024
@jzohrab jzohrab moved this to In Progress in Lute-v3 Nov 1, 2024
@jzohrab
Copy link
Collaborator Author

jzohrab commented Nov 2, 2024

phew! The db is now only imported in the models, flask cli jobs, and flask routes. This was a fair amount of code thrash, but whatever, I felt it was necessary. Going to release it as a beta later, just for me and others to try out.

@jzohrab
Copy link
Collaborator Author

jzohrab commented Nov 16, 2024

In 3.6.0 release.

@jzohrab jzohrab closed this as completed Nov 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-improvement Make code/structure better good first issue Good for newcomers help wanted Extra attention is needed
Projects
Archived in project
Development

No branches or pull requests

1 participant