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

Change Zeitraum to Zeitspanne, fix tests, change zeitspanne datetime to date and time #745

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

hf-lkosareva
Copy link
Contributor

@hf-lkosareva hf-lkosareva commented Mar 5, 2024

Copy link
Collaborator

@hf-krechan hf-krechan left a comment

Choose a reason for hiding this comment

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

Just a few grammar issues.
But the rest looks great. Thanks :)

CONTRIBUTING.md Outdated Show resolved Hide resolved
src/bo4e/com/ausschreibungsdetail.py Outdated Show resolved Hide resolved
#: Zeitraum, in dem der Abschlag zur Anwendung kommen kann
gueltigkeitszeitraum: Optional[Zeitraum] = None
#: Zeitspanne, in dem der Abschlag zur Anwendung kommen kann
gueltigkeitsZeitspanne: Optional[Zeitspanne] = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
gueltigkeitsZeitspanne: Optional[Zeitspanne] = None
gueltigkeitszeitspanne: Optional[Zeitspanne] = None

Copy link
Contributor

Choose a reason for hiding this comment

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

I trink the casing is still camelcase which we want to avoid?

@@ -40,9 +40,9 @@ class Verbrauch(COM):
#: Gibt die Einheit zum jeweiligen Wert an
einheit: Optional[Mengeneinheit] = None

#: Inklusiver Beginn des Zeitraumes, für den der Verbrauch angegeben wird
#: Inklusiver Beginn des Zeitspannees, für den der Verbrauch angegeben wird
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#: Inklusiver Beginn des Zeitspannees, für den der Verbrauch angegeben wird
#: Inklusiver Beginn der Zeitspanne, für den der Verbrauch angegeben wird

src/bo4e/com/verbrauch.py Outdated Show resolved Hide resolved
src/bo4e/com/vertragskonditionen.py Outdated Show resolved Hide resolved
src/bo4e/com/zaehlwerk.py Outdated Show resolved Hide resolved
@hf-kklein
Copy link
Contributor

Did you forget to push changes which you did locally maybe?

@hf-kklein hf-kklein requested a review from a team April 9, 2024 10:36
@hf-fvesely
Copy link
Contributor

Diesen PR erweitern: gerade besprochen mit Kevin, Tim und Joscha: wir nehmen in der Zeitspanne statt start datetime und ende datetime folgendes:

startdatum dateonly, enddatum dateonly, (start und ende inklusiv)
startuhrzeit timeonly enduhrzeit timeonly, (start inklusiv, ende exklusiv).

(Wenn Datum und Uhrzeit angegeben sind, können sie verbunden weren zu datetime - in der jeweiligen Anwendung)

@hf-fvesely hf-fvesely changed the title Change Zeitraum to Zeitspanne, fix tests Change Zeitraum to Zeitspanne, fix tests, change zeitspanne datetime to date and time Jun 7, 2024
@lord-haffi
Copy link
Collaborator

lord-haffi commented Jun 9, 2024

@hf-fvesely Bzgl. der Frage, ob das mit den JSON-Schemas funktioniert: Es wird auf jeden Fall für JSON-Schemas definiert: https://json-schema.org/understanding-json-schema/reference/string#built-in-formats
Pydantic scheint das auch zu implementieren, zumindest läuft der Test für die Doku durch, welcher auch die JSON-Schemas baut.

@hf-fvesely hf-fvesely requested a review from snsttr June 10, 2024 04:28
Diese Komponente wird zur Abbildung von Zeiträumen in Form von Dauern oder der Angabe von Start und Ende verwendet.
Es muss daher entweder eine Dauer oder ein Zeitraum in Form von Start und Ende angegeben sein
"""
abgabefrist: Optional["Zeitspanne"] = None
Copy link
Contributor

Choose a reason for hiding this comment

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

@snsttr Hier sollten wir nochmal weiterdiskutieren wie eine sinnvolle Abbildung aussieeht. Evtl. Abgabefrist als Zeitpunkt und Bindefrist als Zeitmenge, oder Zeitspanne?

Copy link
Contributor

Choose a reason for hiding this comment

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

Wie gehen wir damit um, wenn Felder sowohl eine Zeitspanne als eine Zeitmenge beinhalten könnten, also so wie die alte COM Zeitraum?

Copy link
Contributor

Choose a reason for hiding this comment

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

Mögliche Lösung: Zeitraum doch noch drinlassen, als deprectaed markeiren, aber für manche Fälle noch drinlassen, wenn es wirklcih beides sein kann.
Alternativ: Felder duplizieren: abgabefristdauer, abgeabfristdatum

Copy link
Collaborator

@lord-haffi lord-haffi Jun 12, 2024

Choose a reason for hiding this comment

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

Wie gehen wir damit um, wenn Felder sowohl eine Zeitspanne als eine Zeitmenge beinhalten könnten, also so wie die alte COM Zeitraum?

Optional["Zeitspanne | Zeitmenge"]? Dafür gibt's doch den Union-type :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Stimmt, in einer Diskussion mit Konstantin kam allerdings die Frage auf, ob das so eine gute Idee ist, da es nicht in allen Sprachen ein Pendant zum Union-type gibt.
Da es ja an mehreren Stellen Fristen gibt, kam mir noch die Idee ein COM Frist zu erstellen, was dann beide Optionen beinhaltet. Was denkt ihr? @lord-haffi @snsttr @hf-kklein @hf-krechan @hf-aschloegl

Copy link
Collaborator

Choose a reason for hiding this comment

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

Naja, in den JSON-Schemas wird der Any-Of type definiert, was genau der Union-type in Python ist. Wie die einzelnen Sprachen das Pattern dann umsetzen, ist ein Problem des Code-Generators imo. Bzw. desjenigen, der das dann implementiert. Da die Typen auch disjunkt sind, sehe ich da zumindest in der Theorie auch kein Problem, da nen Union draus zu machen.
@hf-kklein Was ist deine Meinung dazu?

@hf-kklein
Copy link
Contributor

startdatum dateonly, enddatum dateonly, (start und ende inklusiv)
startuhrzeit timeonly enduhrzeit timeonly, (start inklusiv, ende exklusiv).

Was wir dabei leider verlieren ist der utc offset

@lord-haffi
Copy link
Collaborator

startdatum dateonly, enddatum dateonly, (start und ende inklusiv)
startuhrzeit timeonly enduhrzeit timeonly, (start inklusiv, ende exklusiv).

Was wir dabei leider verlieren ist der utc offset

Alternativ: start: Optional[AwareDatetime | date | time]. Fände ich persönlich sogar besser, als es in mehrere Felder aufzuteilen.

@hf-fvesely
Copy link
Contributor

hf-fvesely commented Jun 17, 2024

Hat sich erledigt, Kevin hat den zweiten unnötigen Link gelöscht.:
@lord-haffi @hf-krechan Ich habe nochmal eine ganz andere Frage: Seit PR #831 haben wir dieses problem: https://github.com/bo4e/BO4E-python/actions/runs/9423154482/job/25960970363?pr=745. Kann man da irgednwie die Regel etwas abschwächen?

@hf-fvesely
Copy link
Contributor

startdatum dateonly, enddatum dateonly, (start und ende inklusiv)
startuhrzeit timeonly enduhrzeit timeonly, (start inklusiv, ende exklusiv).

Was wir dabei leider verlieren ist der utc offset

Alternativ: start: Optional[AwareDatetime | date | time]. Fände ich persönlich sogar besser, als es in mehrere Felder aufzuteilen.

@hf-kklein Das time objekt in Python hat eine optionale Zeitzoneninfo:
grafik
https://docs.python.org/3/library/datetime.html

In JsonSchema kommt es auch bald dazu:
grafik
https://json-schema.org/understanding-json-schema/reference/string#dates-and-times

@lord-haffi Zu dem union type gab es mittlerweile einige stimmen, die sich dagegen ausgesprochen haben, da es in manch anderen Sprache eine unnötige Kompliaktion bei der Nutzung der lib dartstellen könnte, für weniger erfahrene Programmierende.
@hf-kklein da hatte Leon ja oben noch nach deiner Einschätzung gefragt.

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.

5 participants