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

refactoring: split python code into classes + utilities + tools #475

Open
lukasrothenberger opened this issue Dec 20, 2023 · 1 comment
Open
Labels
cleanup Cleanup or refactoring required
Milestone

Comments

@lukasrothenberger
Copy link
Collaborator

lukasrothenberger commented Dec 20, 2023

The current state of the library needs refactoring. I propose to refactor to the following structure:

@lukasrothenberger lukasrothenberger added the cleanup Cleanup or refactoring required label Dec 20, 2023
@lukasrothenberger lukasrothenberger changed the title refactoring: split python code into library( classes + utilities) + tools refactoring: split python code into classes + utilities + tools Dec 20, 2023
@goerlibe
Copy link
Collaborator

Seems good 👍
One suggestion: We move all the command line stuff to a separate directory/package/... .
→ The top-level directories would then be: cli, tools, utilities, classes
Currently the cli functionality is already well-separated from the actual implementation but the separation is not really visible as the files all reside in the same directory.

We could even take it even one step further and have completely separate packages for each tool. e.g.

main package for a "standard installation"

  • discopop (requires all the cli packages. Installing discopop therefore results in a "full" installation of everything)

cli packages

  • discopop-explorer-cli (requires discopop-explorer)
  • discopop-optimizer-cli (requires discopop-optimizer)
  • ...

pure python packages that provide the python APIs

They each require the discopop-common package, if needed. If a component is however only needed in one package then it can reside in the respective package.

  • discopop-explorer
  • discopop-optimizer
  • ...

all the shared stuff goes into discopop-common

  • discopop-common
    • utilities
    • classes

In theory this would make it possible to install only the tools needed for the job at hand (+ discopop-common), e.g. if a user is not interested in the optimizer or the patches, only the discopop-explorer(-cli) would need to be installed.
While this is not really a common use case, it would force us into separating concerns more strictly, making the code more reusable and easier to understand.

A downside I can think of: having separate packages for each tool would require us to make sure that installed versions match properly. However this is also something I would also like to see from the VSCX perspective. Currently there is no check to ensure that the installed discopop version is compatible with the VSCX extension. I feel like a notification should show up during startup to let the user know if the (oftentimes automatically updated) extension is intended to work with a different discopop version.

PS:
The discopop-profiler is completely outdated and can be removed, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Cleanup or refactoring required
Projects
None yet
Development

No branches or pull requests

2 participants