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

Ddb/add autogenerated models #23

Merged
merged 72 commits into from
Apr 30, 2024
Merged

Conversation

DeltaDaniel
Copy link
Collaborator

No description provided.

tox.ini Show resolved Hide resolved
tox.ini Show resolved Hide resolved
@DeltaDaniel DeltaDaniel requested a review from hf-kklein January 29, 2024 09:15
Copy link
Contributor

@hf-kklein hf-kklein left a comment

Choose a reason for hiding this comment

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

Ich steck nicht tief drin aber es wirkt auf mich ganz stimmig.

zwei sachen habe ich:

  • bei den tests die cleanup-parts bitte in die fixture verlagern, sodass sie auch ausgeführt werden, wenn der test nicht erfolgreich läuft
  • die version/tag/commit von den via git installierten tools pinnen (sonst hat man keine reproduzierbaren ergebnisse)


def main() -> None:
"""
collects all fcts to be executed when running borm.
Copy link
Contributor

Choose a reason for hiding this comment

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

was sind fcts?

Comment on lines 8 to 12
import borm.models # noqa # pylint: disable=unused-import

# Import all the models, so that Base has them before being
# imported by Alembic
from borm.db.base_class import MappingBase, mapper_registry
from borm.db.base_class import MappingBase, mapper_registry # noqa # pylint: disable=unused-import
Copy link
Contributor

Choose a reason for hiding this comment

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

kannst du dazuschreiben, warum der import notwendig ist? 8ws. weil wir unten die dinge dynamisch importen?

tox.ini Outdated
Comment on lines 120 to 121
BO4E-Schema-Tool @ git+https://github.com/bo4e/BO4E-Schema-Tool.git
BO4E-Python-Generator @ git+https://github.com/bo4e/BO4E-Python-Generator.git@DDB/create_SQLModels
Copy link
Contributor

Choose a reason for hiding this comment

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

kannst du diese dependencies bitte in eine datei stecken und pinnen? sodass sie nicht auf einen branch verweisen sondern auf einen commit oder tag?
am besten in einer requireqments.txt damit dependabot das bumpen kann

Comment on lines 79 to 83
session.delete(testgeschaeftspartner1)
session.delete(testgeschaeftspartner2)
session.delete(testangebot1)
session.delete(testangebot2)
session.commit()
Copy link
Contributor

Choose a reason for hiding this comment

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

ist das ein cleanup nach dem test, damit er bemi zweiten mal auch noch läuft?
falls ja: besser wäre du baust die initialize_sessoin fixture so um, dass sie eine yield-fixture ist und nach dem yield machst du den cleanup. so würde ja das delete nicht ausgeführt, wenn vorher was crasht. und dann hast du einen inkonsistenten, nicht reproduzierbaren zustand.

Comment on lines 117 to 120
session.delete(testgeschaeftspartner1)
session.delete(testgeschaeftspartner2)
session.delete(testangebot1)
session.commit()
Copy link
Contributor

Choose a reason for hiding this comment

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

cleanup bitt in die fixture

Copy link
Contributor

@hf-kklein hf-kklein left a comment

Choose a reason for hiding this comment

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

die dinge, die sich seit letztem mal geändert haben (github "changed since last view" sehen sauber aus. lass uns mal nachher drüber sprechn, was wir jetzt damit machen :)



@pytest.fixture(scope="function")
def initialize_session(): # type: ignore[no-untyped-def]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def initialize_session(): # type: ignore[no-untyped-def]
def initialize_session()->Generator[Session, None, None]:

tox.ini Outdated
#unused imports
#lines in comments too long
#too many lines -> many-many relationship classes
pylint src/borm/models --disable R0801,C0114,C0115,W0611,C0301,C0302
Copy link
Contributor

Choose a reason for hiding this comment

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

wäre es nicht einfacher, das auch in eine .pylintrc zu schreiben?

@DeltaDaniel DeltaDaniel merged commit fb32617 into main Apr 30, 2024
15 checks passed
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