refactor: introduce new internal representation for Opossum files #192
+357
−292
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
abraemer
force-pushed
the
refactor-consistent-model-pipeline
branch
2 times, most recently
from
1e1d053 to
057727c
Compare
abraemer
force-pushed
the
refactor-consistent-model-pipeline
branch
2 times, most recently
from
60b1e47 to
59f5dc7
Compare
Hellgartner
reviewed
Jan 22, 2025
abraemer
force-pushed
the
refactor-consistent-model-pipeline
branch
from
54f5c61 to
bd3735f
Compare
|
Note: rebased onto current main |
mstykow
reviewed
Jan 22, 2025
src/opossum_lib/opossum_model.py
Outdated
| ) | ||
| output_file: OpossumOutputFile | None = None | ||
|
|
||
| def to_opossum_file_format(self) -> opossum_file_content.OpossumFileContent: |
There was a problem hiding this comment.
i think it would be nicer to read if at the top of the file you don't use namespace imports but import exactly the classes you need.
There was a problem hiding this comment.
There is some duplication of names between opossum_file and opossum_model. That's why I wanted to explicit where which name comes from.
This duplication comes from the fact that opossum_model is quite similar to opossum_file generally but we chose to separate the implementations which meant copying some models verbatim.
The specific instance you marked could be removed though as there are only clashes between opossum_file and opossum_model.
Hellgartner
requested changes
Jan 22, 2025
* this model encapsulates also the semantic relationships of resources, resourcesToAttribuions and externalAttributions. These are not enforced by the file structure alone. * This will be used as a target for the other file format frontends and simplify their logic. * It also allows for easier testing since it allows to check for semantic/structural equivalence among opossum files (e.g. the IDs of the attribution carry no semantic semantic information themselves i.e. are arbitrary labels)
* Opossum class should be able to carry all information that could be present in an .opossum file
* resources can now by added to the resource structure without knowledge about internals * for this reason: - resources can be created with just a path (i.e. without type) - resources can now be merged together if the types are compatible. types are compatible if at least one is not set or types are identical - when converting to opossum file format, unset type defaults to folder. Maybe this should raise an error instead, but being more permissible probably just makes things more ergonomic without hurting correctness.
* default to treating a Resource as file if the type is undefined and no children present * provide slightly more information in an error message
abraemer
force-pushed
the
refactor-consistent-model-pipeline
branch
from
bd3735f to
60f4f48
Compare
Hellgartner
approved these changes
Jan 23, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of changes
This PR does not contain tests for the new model. These are done separately see #189
Context and reason for change
See: #190
Closes: #195
Part1 of: #196 (tests come in a follow-up because they require #194 )