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

Finalize switch flow #207

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

Finalize switch flow #207

wants to merge 51 commits into from

Conversation

Hellgartner
Copy link
Contributor

Closes #199

Goal is to move to a global flow which is

  • obvious to follow
  • easily extendable
  • serves as a prestep for merging

Note: Added a converter class for scancode to
Opossum to allow for individual testing
* Resolved conflicts src/opossum_lib/cli.py
* Needed to adapt (as this now contains functionality
  which has been in cli.py)
   * src/opossum_lib/core/input_file.py
   * src/opossum_lib/core/opossum_generation_arguments.py
   * src/opossum_lib/core/opossum_generator.py
Goal: The different file formats should not depend on each other
This allows to write a rule for import linter
which automatically extends on new packages
@Hellgartner Hellgartner marked this pull request as ready for review January 28, 2025 11:42
src/opossum_lib/core/input_reader.py Outdated Show resolved Hide resolved
src/opossum_lib/core/opossum_generation_arguments.py Outdated Show resolved Hide resolved
src/opossum_lib/core/opossum_generation_arguments.py Outdated Show resolved Hide resolved
src/opossum_lib/core/input_file.py Outdated Show resolved Hide resolved
@mstykow mstykow self-assigned this Jan 28, 2025
* Remove the warning log if the output file is overwritten
* It is not useful for the user as it she/he cannot react on the message
* Instead, put a clearer warning in the docs + help message
README.md Outdated Show resolved Hide resolved
src/opossum_lib/core/opossum_generation_arguments.py Outdated Show resolved Hide resolved
src/opossum_lib/core/opossum_generation_arguments.py Outdated Show resolved Hide resolved
src/opossum_lib/core/opossum_generator.py Outdated Show resolved Hide resolved
src/opossum_lib/input_formats/scancode/model.py Outdated Show resolved Hide resolved
@Hellgartner Hellgartner requested a review from mstykow January 29, 2025 16:44
COMPRESSION_LEVEL,
INPUT_JSON_NAME,
OUTPUT_JSON_NAME,
)
from opossum_lib.opossum.opossum_file_content import OpossumFileContent
from opossum_lib.shared.entities.opossum_file_model import OpossumFileModel


class OpossumFileWriter:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these classes that only contain @staticmethods are not idiomatic in Python.
You get the very same behavior if you make the methods simply top-level methods of the module. I'd prefer removing the unnecessary classes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I'd propose a different split into functions:

def _dump_to_json(model: BaseModel) -> str:
    model.model_dump_json(exclude_none=True, indent=4, by_alias=True)

def write(opossum_file_model: OpossumFileModel, file_path: Path) -> None:
    file_path = file_path.with_suffix(".opossum")
    with ZipFile(file_path, "w", compression=ZIP_DEFLATED, compresslevel=COMPRESSION_LEVEL) as zip_file:
        zip_file.writestr(INPUT_JSON_NAME, _dump_to_json(opossum_file_model.input_file))
        if opossum_file_model.output_file:
            zip_file.writestr(OUTPUT_JSON_NAME, _dump_to_json(opossum_file_model.output_file))

Which I find much more readable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think these classes that only contain @staticmethods are not idiomatic in Python.
You get the very same behavior if you make the methods simply top-level methods of the module. I'd prefer removing the unnecessary classes.

When syncing with @mstykow we agreed on explicilty having these classes so I would like to keep them

Copy link
Contributor

Choose a reason for hiding this comment

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

ok. Let's keep these then. What do you think about the different organization of methods?


return ScancodeDataToOpossumConverter.convert_scancode_to_opossum(scancode_data)

@staticmethod
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this a static method?
If it is internal, then use an "_" (and make it an instance method to hide it even more). If it is API then it can just be a top-level method.


class ScancodeDataToOpossumConverter:
@staticmethod
def convert_scancode_to_opossum(scancode_data: ScanCodeDataModel) -> Opossum:
Copy link
Contributor

@abraemer abraemer Jan 30, 2025

Choose a reason for hiding this comment

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

I think only this method is public and the rest should be internal -> prefix with "_"
Consider removing the wrapping class as the namespacing gets a bit too much: SCANCODE.services.SCANCODE_data_to_opossum_converter.SCANCODEDataToOpossumConverter.convert_SCANCODE_to_opossum (emphasis on how often scancode appears)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see above



class ScanCodeData(BaseModel):
class ScanCodeDataModel(BaseModel):
Copy link
Member

Choose a reason for hiding this comment

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

scan_code -> scancode and instead of "data" perhaps "file"

Suggested change
class ScanCodeDataModel(BaseModel):
class ScancodeFileModel(BaseModel):

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.

Final: Switch flow
3 participants