Pull database logic out of core.py and transaction.py #62
Conversation
| @@ -8,7 +8,6 @@ services: | |||
| build: | |||
| context: . | |||
| dockerfile: Dockerfile | |||
| platform: linux/amd64 | |||
There was a problem hiding this comment.
My linux laptop doesn't like this line?
There was a problem hiding this comment.
interesting, i'm running on macos so probably why i didn't see it
|
I've only moved the db logic out of core.py so far |
| @@ -51,23 +41,14 @@ class CoreCrudClient(BaseCoreClient): | |||
| ) | |||
| settings = ElasticsearchSettings() | |||
| client = settings.create_client | |||
| database = CoreDatabaseLogic() | |||
There was a problem hiding this comment.
It looks like this database attribute completely abstracts away access to the client and settings, is that correct? If so, the client and settings attributes should be removed.
Also, is there any reason not to just call it Database instead of CoreDatabaseLogic?
There was a problem hiding this comment.
True - good catch. I was going to have a TransactionsDatabaseLogic but it could all be one class or have a better name too.
There was a problem hiding this comment.
+1 for just having one class for both.
perhaps DatabaseClient? I assume the longer-term goal here is to have ElasticsearchDatabaseClient and OpensearchDatabaseClient, that abstract away the small differences between the two.
There was a problem hiding this comment.
I could also have a MongoClient outside of the repo that I could maybe inject when I wanted to and not have to keep a full fast-stac-api backend up to date.
| except elasticsearch.exceptions.NotFoundError: | ||
| raise NotFoundError("No collections exist") |
There was a problem hiding this comment.
Should this return an empty Collections instead of raising?
There was a problem hiding this comment.
True, that would make sense. I would have to ask @philvarner maybe.
There was a problem hiding this comment.
yes, an empty Collections rather than 404
There was a problem hiding this comment.
maybe file an issue and put that in a separate PR instead of this one -- just focus on the reorganization here.
|
@jonhealy1 what's your ETA on having this ready to merge? since it's blocking any other work being done because it moves so much code around |
I can finish tomorrow - I'm in Turkey so it's 12:06 am here right now. Or we could merge this and make a pr to do the transactions code later if you think? |
|
@philvarner sorry for blocking you - this should be ready now |
|
Just some final cleaning .... and a ridiculous ci error |
no worries, thanks for getting this done so quickly! |
Related Issue(s):
Description:
Pull db logic out of core and transaction.py to make codebase more modular.
PR Checklist:
pre-commit run --all-files)make test)make docs)