Don't subclass RecData for RecipeManager #125
Conversation
There was a problem hiding this comment.
These are fairly in-depth changes, but that part of the application is heavily used, and from using the changes and perusing the code, it looks good!
Additional thumbs-up for the clear comments in RecipeManager.__getattr__.
The only minor comment I have are to break the long lines no. 1965 and 1967 in gourmet/backends/db.py.
gourmet/backends/db.py
Outdated
| @@ -114,7 +114,15 @@ class DBObject: | |||
| # categories_table: id -> recipe_id, category_entry_id -> id | |||
| # ingredients_table: ingredient_id -> id, id -> recipe_id | |||
|
|
|||
| class RecData (Pluggable, BaseException): | |||
| def db_url(file=None, custom_url=None): | |||
There was a problem hiding this comment.
please add type annotations while you're at it
gourmet/backends/db.py
Outdated
| _instance_by_db_url = {} | ||
|
|
||
| @classmethod | ||
| def instance_for(cls, file=None, custom_url=None): |
There was a problem hiding this comment.
please add type annotations to this method
gourmet/backends/db.py
Outdated
| custom_url=None): | ||
| return cls._instance_by_db_url[url] | ||
|
|
||
| def __init__ (self, file, url): |
There was a problem hiding this comment.
please add type annotations to this method
gourmet/backends/db.py
Outdated
|
|
||
| def __init__ (self,*args,**kwargs): | ||
| @classmethod | ||
| def instance_for(cls, file=None, custom_url=None): |
There was a problem hiding this comment.
please add type annotations to this method
|
I kind of feel that the |
We will thank ourselves once we do any more complex refactoring. It looks good to me. Tank you. |
|
Thanks again :) |
RecipeManager being a subclass of RecData was causing problems for the singleton-ish machinery: if the single RecData instance was set up before RecipeManager, code that needed a RecipeManager object could get a RecData object instead. This aims to fix that, by moving from inheritance to composition.
Of course, lots of code uses methods from the (former) parent class on a RecipeManager object. Rather than trying to find and modify every case of that, I've added a
__getattr__method so it will still work.I've also understood & cleaned up the logic for the not-quite-singleton machinery in RecData, and replicated it for RecipeManager. The idea is to have one instance of each per SQLite database file (or per SQLalchemy database URL).