Add function to get EdifactFormat and Version of files, with tests #124

hf-sheese · 2024-01-30T15:02:59Z

In order to sort files into format version specific folders, a function determining EdifactFormatVersion and EdifactFormat with tests is added.

hf-kklein · 2024-01-30T15:14:53Z

Die alten Tests failen, weil die Foo | None Syntax erst ab 3.10(?) eingeführt wurde. also du müsstest auf die abwärtskopmatible Optional[Foo] zurückgreifen.

hf-kklein

Ist schon schön test-driven :) Ein paar Sachen - v.a. die Testparametrisierung können wir noch ein bisschen besser machen

unittests/test_edienergyscraper.py

src/edi_energy_scraper/__init__.py

…o `list[...]` nor `tuple[...]`) (#125) * is tuple the problem? * so? * just to be sure? * Revert "just to be sure?" This reverts commit f0c178c. * isort

…mat_version

hf-kklein

Passt für mich im großen und ganzen.

Allerdings fände ich es schöner, wenn wir leere listen statt [None] zurückgeben.

Dann würdest du dir sogar das Typing vereinfachen, weil der type dann nur noch list[EdifactFormat] ist statt list[Optional[EdifactFormat]] und man downstream nur noch prüfen muss, ob die liste einträge hat anstatt zu checken: hat die liste Einträage und sind diese Einträge nicht None.

Ich glaube das ist so gewachsen, weil wir erst dachten, es gäbe eines oder kein EdifactFormat (also nicht-None oder None). Dann hast du entdeckt, dass die CONTRL/APERAK AHBs mehrere Formaten zuzuordnen sind und wir sind auf eine Liste gewechselt. Mit der liste ist die Unterscheidung "keines/eines/mehrere" aber nicht mehr eine Entscheidung zwischen nicht-None und None, sondern wird einfch durch die länge der liste repräsentiert.

@hf-krechan was meinst du?

hf-kklein · 2024-01-31T11:16:26Z

src/edi_energy_scraper/__init__.py

+        edifactformat: List[Optional[EdifactFormat]] = []
+        for entry in EdifactFormat:
+            if str(entry) in filename:
+                edifactformat.append(entry)
+        if not edifactformat:
+            edifactformat = [None]
+        return version, edifactformat


Suggested change

edifactformat: List[Optional[EdifactFormat]] = []

for entry in EdifactFormat:

if str(entry) in filename:

edifactformat.append(entry)

if not edifactformat:

edifactformat = [None]

return version, edifactformat

edifactformats: List[Optional[EdifactFormat]] = []

for entry in EdifactFormat:

if str(entry) in filename:

edifactformats.append(entry)

if not any(edifactformats):

edifactformats = [None]

return version, edifactformats

plural dinge (wie listen) sollen plural namen haben

ahja, oder so. Wie gesagt, nur plural S überlese ich leicht

hf-kklein · 2024-01-31T11:16:53Z

src/edi_energy_scraper/__init__.py

@@ -301,6 +303,27 @@ async def _download(
            raise
        return file_path

+    @staticmethod


wenn es eine static-method ist, dann kannst du auch gleich eine function draus machen (die anders als methods) keiner klasse angehört.

hf-kklein · 2024-01-31T11:17:18Z

src/edi_energy_scraper/__init__.py

@@ -301,6 +303,27 @@ async def _download(
            raise
        return file_path

+    @staticmethod
+    def get_edifact_format(path: Path) -> Tuple[EdifactFormatVersion, List[Optional[EdifactFormat]]]:


vllt edifact_formats (plural) and version? wobei der return-ype hint ja schon viel verrät.

tjaja good old naming
vielleicht so get_edifact_version_and_formats

hf-kklein · 2024-01-31T11:19:15Z

src/edi_energy_scraper/__init__.py

+        if not edifactformat:
+            edifactformat = [None]


Das versteh ich nicht: warum adden wir None zur liste anstatt sie leer zu lassen?
Nach außen würde doch genügen, wenn wir eine leere liste haben anstatt eine liste mit None als einzigem Eintrag.
Für mich drückt eine leere liste genau das aus, was es ist: die datei kann len(formats)==0 formaten zugeordnet werden.

das None als einziger Eintrag scheint mir doppelt gemoppelt/nur zum selbstzweck.

ja würde ich auch rausnehmen. Spart auch zwei Zeilen Code :)

hf-krechan · 2024-01-31T12:27:43Z

src/edi_energy_scraper/__init__.py

+        berlin = pytz.timezone("Europe/Berlin")
+        berlin_local_time = datetime.datetime.strptime(date_string, date_format).astimezone(berlin)
+        version = get_edifact_format_version(berlin_local_time)
+        edifactformat: List[Optional[EdifactFormat]] = []


ich finde es angenehm wenn ich der variable ansehe, ob es sich um eine Liste/Array oder etwas Einzelnes handelt.
list_of_edifactformats . Reines Plural-S übersieht man leicht.

hf-krechan

Sehr schöne Tests. Das mit dem None in der Liste würde ich noch rausmachen und das Plural bei der Liste.
Sonst passt das für mich.
Vielen lieben Dank 👍

✅Add function to get edifactformat and version of files, with tests

e53edf3

hf-sheese requested review from hf-kklein and hf-krechan January 30, 2024 15:03

hf-kklein reviewed Jan 30, 2024

View reviewed changes

hf-sheese and others added 10 commits January 30, 2024 16:44

✅Increase compatibility for older python versions

6566d85

📝Add docstring for test

9e4b9c0

🚧 Compatibility with Pyton 3.8

14cce33

🚧Compatibility with Python 3.8

77ac623

Use upper case List[...] and Tuple[...] (because Python 3.8 has n…

e5d167e

…o `list[...]` nor `tuple[...]`) (#125) * is tuple the problem? * so? * just to be sure? * Revert "just to be sure?" This reverts commit f0c178c. * isort

Merge remote-tracking branch 'origin/main' into determine_edifact_for…

a3c2825

…mat_version

parametrize correctly

f91af36

upper case fun

be7b97c

Interprete date-Strings als berliner Lokalzeit (ME(S)Z)

b50f7f2

✅Improve parameterization of tests

6d86e8c

hf-sheese requested a review from hf-kklein January 31, 2024 11:08

hf-kklein changed the title ~~Add function to get edifactformat and version of files, with tests~~ Add function to get EdifactFormat and Version of files, with tests Jan 31, 2024

hf-kklein approved these changes Jan 31, 2024

View reviewed changes

hf-kklein assigned hf-krechan and hf-sheese Jan 31, 2024

hf-krechan reviewed Jan 31, 2024

View reviewed changes

hf-krechan approved these changes Jan 31, 2024

View reviewed changes

Empty list (instead of None) if file contains no format

1eb67ae

hf-sheese merged commit b6f72e4 into main Jan 31, 2024
23 checks passed

hf-sheese deleted the determine_edifact_format_version branch January 31, 2024 12:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add function to get EdifactFormat and Version of files, with tests #124

Add function to get EdifactFormat and Version of files, with tests #124

hf-sheese commented Jan 30, 2024

hf-kklein commented Jan 30, 2024 •

edited

Loading

hf-kklein left a comment

hf-kklein left a comment

hf-kklein Jan 31, 2024

hf-krechan Jan 31, 2024

hf-kklein Jan 31, 2024

hf-kklein Jan 31, 2024

hf-krechan Jan 31, 2024 •

edited

Loading

hf-kklein Jan 31, 2024

hf-krechan Jan 31, 2024

hf-krechan Jan 31, 2024

hf-krechan left a comment

Add function to get EdifactFormat and Version of files, with tests #124

Add function to get EdifactFormat and Version of files, with tests #124

Conversation

hf-sheese commented Jan 30, 2024

hf-kklein commented Jan 30, 2024 • edited Loading

hf-kklein left a comment

Choose a reason for hiding this comment

hf-kklein left a comment

Choose a reason for hiding this comment

hf-kklein Jan 31, 2024

Choose a reason for hiding this comment

hf-krechan Jan 31, 2024

Choose a reason for hiding this comment

hf-kklein Jan 31, 2024

Choose a reason for hiding this comment

hf-kklein Jan 31, 2024

Choose a reason for hiding this comment

hf-krechan Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

hf-kklein Jan 31, 2024

Choose a reason for hiding this comment

hf-krechan Jan 31, 2024

Choose a reason for hiding this comment

hf-krechan Jan 31, 2024

Choose a reason for hiding this comment

hf-krechan left a comment

Choose a reason for hiding this comment

hf-kklein commented Jan 30, 2024 •

edited

Loading

hf-krechan Jan 31, 2024 •

edited

Loading