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

🔀 MERGE: Refactor/rename storage modules #5364

Merged
merged 4 commits into from
Feb 21, 2022

Conversation

chrisjsewell
Copy link
Member

@chrisjsewell chrisjsewell commented Feb 17, 2022

This is a follow-up to #5330

Previously, I believe this may not have been possible,
due to django loading the database connection on install.
But now, it makes sense to consolidate the full psql_dos
storage backend implementation in a central place.
This also leaves aiida/orm fully storage backend agnostic.

@chrisjsewell chrisjsewell requested a review from sphuber February 17, 2022 22:57
@chrisjsewell
Copy link
Member Author

tiny diff lol

@chrisjsewell
Copy link
Member Author

P.S. obviously, the next thing will be to rename sqlalchemy to psql_dos

@chrisjsewell
Copy link
Member Author

the next thing will be to rename sqlalchemy to psql_dos

Was too impatient, done

@chrisjsewell chrisjsewell force-pushed the move-sqla-orm branch 2 times, most recently from 47024d7 to dd46ee2 Compare February 18, 2022 06:15
@chrisjsewell
Copy link
Member Author

chrisjsewell commented Feb 18, 2022

Note, the docs require #5365 to fix

…orm`

Previously, this was possible,
due to django loading the database connection on install.
But now, the full `psql_dos` storage backend implementation
can be consolidated in a single module.
This brings it inline with the actual backend name
@sphuber
Copy link
Contributor

sphuber commented Feb 18, 2022

While we are in the process of renaming things as they should be named, should aiida/backends become aiida/storage?

@chrisjsewell
Copy link
Member Author

While we are in the process of renaming things as they should be named, should aiida/backends become aiida/storage?

Yeh was thinking that, can do it in another commit here?

Then, there are other uses of "backend":

  • should the aiida/orm/implementation/backends.py::Backend class be re-named?
  • you have things like Entity.backend, but that's a bit more difficult to change

@sphuber
Copy link
Contributor

sphuber commented Feb 18, 2022

Yeh was thinking that, can do it in another commit here?

Fine by me to do it in this PR, but not sure if these need to be separate commits. Well, at least the two you have now can really be squashed because it is the same folder being renamed twice in sequence:

  1. aiida.orm.implementation.sqlalchemy - > aiida.backends.sqlalchemy.orm
  2. aiida.backends.sqlalchemy.orm -> aiida.backends.psql_dos.orm

Is really the same as

  1. aiida.orm.implementation.sqlalchemy - > aiida.backends.psql_dos.orm

The next move of aiida.backends -> aiida.storage maybe should indeed better be a separate commit, since it involves the renaming of more modules.

should the aiida/orm/implementation/backends.py::Backend class be re-named?

I don't think the name backend is problematic in itself, but for consistency it should probably be storage_backend/StorageBackend.

you have things like Entity.backend, but that's a bit more difficult to change

Why is this more difficult? From a backwards-incompatible perspective? Because if it is that, users are not supposed to be touching this anyway, right?

@chrisjsewell
Copy link
Member Author

Well, at least the two you have now can really be squashed because it is the same folder being renamed twice in sequence
Is really the same as aiida.orm.implementation.sqlalchemy - > aiida.backends.psql_dos.orm

I would not say that is equivalent; it's a movement of some code between modules, then a name change of an entire module.

@chrisjsewell
Copy link
Member Author

Why is this more difficult? From a backwards-incompatible perspective? Because if it is that, users are not supposed to be touching this anyway, right?

Yeh, although we don't "intend" users to touch it, Entity.backend is still a public attribute.
and just making sure everything is correctly updated, e.g. things like orm.QueryBuilder(backend=self.backend)

@chrisjsewell
Copy link
Member Author

chrisjsewell commented Feb 18, 2022

I don't think the name backend is problematic in itself

it mainly just not particularly descriptive and/or immediately obvious to new users/developers what it means

@chrisjsewell chrisjsewell changed the title ♻️ REFACTOR: orm/implementation/sqlalchemy -> backends/sqlalchemy/orm 🔀 MERGE: Refactor/rename storage modules Feb 18, 2022
@sphuber
Copy link
Contributor

sphuber commented Feb 18, 2022

it mainly just not particularly descriptive and/or immediately obvious to new users/developers what it means

I meant that I don't think the word "backend" is problematic, however, when it is isolated, it loses meaning. But when combined with a word indicating what it is actually the backend of, such as storage_backend, then I think it is fine and conveys the correct message.

@chrisjsewell
Copy link
Member Author

chrisjsewell commented Feb 18, 2022

But when combined with a word indicating what it is actually the backend ... then I think it is fine and conveys the correct message.

yep 👍

@chrisjsewell
Copy link
Member Author

Ok @sphuber, done what I'm going to do in this PR

@chrisjsewell
Copy link
Member Author

Ok I'm going to be merging this soon, since I have other PRs to go on top

@chrisjsewell chrisjsewell merged commit 250dd96 into aiidateam:develop Feb 21, 2022
@chrisjsewell chrisjsewell deleted the move-sqla-orm branch February 21, 2022 12:54
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.

2 participants