Skip to content

Tests for HermesContext and HermesCache - #382

Merged
SKernchen merged 8 commits into
refactor/data-modelfrom
refactor/366-refactor-current-model-tests
Jul 28, 2025
Merged

Tests for HermesContext and HermesCache#382
SKernchen merged 8 commits into
refactor/data-modelfrom
refactor/366-refactor-current-model-tests

Conversation

@SKernchen

Copy link
Copy Markdown
Contributor

No description provided.

@SKernchen
SKernchen requested a review from sdruskat July 11, 2025 08:22

@led02 led02 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice and well collected test cases, also very pythonic!

See inline comments for minor changes.

Comment thread test/hermes_test/model/test_base_context.py Outdated
Comment thread test/hermes_test/model/test_base_context.py
Comment on lines -1 to -3
# SPDX-FileCopyrightText: 2022 German Aerospace Center (DLR)
#
# SPDX-License-Identifier: Apache-2.0

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think I added this because REUSE complained.
I agree that empty files have not much originalty in it, so on could argue whether we need this license tag.
However, we should at least be consistent here...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes we can keep it, I think it changed automatically, because of recent open files.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The FAQs suggest that it's fine to use either the default copyright, or CC0-1.0 for license and NONE for copyright holder. So I think that keeping as is is fine.

SKernchen and others added 2 commits July 14, 2025 13:04
Co-authored-by: Michael Meinel <michael.meinel@dlr.de>
Comment thread src/hermes/model/context_manager.py Outdated
Co-authored-by: Stephan Druskat <sdruskat@users.noreply.github.com>

@led02 led02 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor fixes which can be addressed in other referenced issues.

Comment thread src/hermes/model/context_manager.py Outdated
Comment on lines +61 to +64
current_step = self._current_step[-1]
if current_step != step:
raise ValueError(f"Cannot end step {step} while in {self._current_step[-1]}.")
self._current_step.pop()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do you put the "fix" into the comment instead of putting it into the implementation?

The however, the first line will still throw an IndexError for empty lists.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Documented in #373.

Comment thread src/hermes/model/context_manager.py Outdated
Comment on lines +71 to +72


Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

PEP8-Violation 🚨

@SKernchen

Copy link
Copy Markdown
Contributor Author

Last commit would close #373 and #375

Comment thread src/hermes/model/context_manager.py Outdated
Co-authored-by: Michael Meinel <michael.meinel@dlr.de>
@SKernchen
SKernchen merged commit a77ba6b into refactor/data-model Jul 28, 2025
1 of 5 checks passed
@SKernchen
SKernchen deleted the refactor/366-refactor-current-model-tests branch July 28, 2025 11:00
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.

3 participants