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

Memorize which File contains which Pruefi Tables #209

Merged
merged 11 commits into from
Jan 19, 2024

Conversation

hf-sheese
Copy link
Contributor

@hf-sheese hf-sheese commented Dec 22, 2023

To speed up the process of looking for pruefis, the routine to collect all pruefis is altered to also save the name of the file it found the pruefi in. This information is then used to only look through only one file per pruefi, speeding up the process of getting all existing pruefis.
Successfully merging this pull request may close these issues. #148

collect_pruefis.py collect name of file where pruefi is found in. get_all_pruefis then only looks through that file, which speeds up the search for all pruefis.
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.

Super, dass wir jetzt bessere Performance haben (haben wir doch?)

  • Ich würde mir noch wünschen: dass "kein Erfolg" lieber als None/null denn als empty string abgebildet wird.
  • Und etwas mehr doku über das dict[str,str] ;)
  • und die bedeutung der toml-datei

src/kohlrahbi/__init__.py Outdated Show resolved Hide resolved
src/kohlrahbi/__init__.py Outdated Show resolved Hide resolved
src/kohlrahbi/__init__.py Outdated Show resolved Hide resolved
src/kohlrahbi/__init__.py Outdated Show resolved Hide resolved
src/kohlrahbi/__init__.py Outdated Show resolved Hide resolved
src/kohlrahbi/all_known_pruefis.toml Show resolved Hide resolved
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.

Passt für mich. bitte

  • verwende Optional[str] oder str|None anstatt Union[str, None]
  • überlege, ob an der einen Stelle nicht auch noch der Type Path besser wäre als str
  • kommentier an einer stelle noch was keys/values sind
  • lass kevin noch drüber schauen
  • überprüfe ob die README.md noch aktuell ist was die benutzung angeht

src/kohlrahbi/__init__.py Outdated Show resolved Hide resolved
src/kohlrahbi/__init__.py Outdated Show resolved Hide resolved
src/kohlrahbi/__init__.py Outdated Show resolved Hide resolved
src/kohlrahbi/__init__.py Outdated Show resolved Hide resolved
src/kohlrahbi/__init__.py Outdated Show resolved Hide resolved
src/kohlrahbi/__init__.py Outdated Show resolved Hide resolved
src/kohlrahbi/__init__.py Outdated Show resolved Hide resolved
src/kohlrahbi/collect_pruefis.py Outdated Show resolved Hide resolved
src/kohlrahbi/collect_pruefis.py Show resolved Hide resolved
unittests/test_input_checks.py Outdated Show resolved Hide resolved
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.

Sieht schon mal ganz gut aus.
Würde die Funktion get_valid_pruefi und die Zurodnung Prüfi zu Dateipfad anders schneiden falls das geht.

src/kohlrahbi/__init__.py Outdated Show resolved Hide resolved
src/kohlrahbi/__init__.py Outdated Show resolved Hide resolved
src/kohlrahbi/__init__.py Outdated Show resolved Hide resolved
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.

wenn du die Kleinigkeiten noch einarbeitest kannst du es gerne mergen :)
vielen lieben dank für deine Arbeit

src/kohlrahbi/__init__.py Outdated Show resolved Hide resolved
src/kohlrahbi/__init__.py Outdated Show resolved Hide resolved
src/kohlrahbi/all_known_pruefis.toml Outdated Show resolved Hide resolved
src/kohlrahbi/__init__.py Outdated Show resolved Hide resolved
src/kohlrahbi/__init__.py Outdated Show resolved Hide resolved
@@ -259,11 +266,11 @@ def process_pruefi(
for ahb_file_path in ahb_file_paths:
doc = get_or_cache_document(ahb_file_path, path_to_document_mapping)
Copy link
Collaborator

Choose a reason for hiding this comment

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

ist in dem Repo out of scope, aber die Funktion tut zwei Dinge was sie auch im Namen schon verrät.
Wäre cooler wenn das entsprechend zwei Funktionen sind. Aber das ist für einen anderen PR ^^

src/kohlrahbi/__init__.py Outdated Show resolved Hide resolved
edi_energy_mirror_repo_root_path / Path("edi_energy_de/current"),
edi_energy_mirror_repo_root_path / Path("edi_energy_de/future"),
edi_energy_mirror_repo_root_path
/ Path("edi_energy_de/current"), # If you are interested in coming changes, replace "current" with "future"
Copy link
Collaborator

Choose a reason for hiding this comment

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

das will ich in Zukunft mit einer flag regeln

@hf-sheese hf-sheese merged commit e9b66d4 into main Jan 19, 2024
8 checks passed
@hf-sheese hf-sheese deleted the memorize-pruefi-filename branch January 19, 2024 14:32
@hf-krechan hf-krechan linked an issue Jan 20, 2024 that may be closed by this pull request
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.

Memorize which File contains which Pruefi Tables
3 participants