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

Import modules in OCI.new() explicitly #2328

Merged
merged 1 commit into from
Jul 25, 2023
Merged

Conversation

dcermak
Copy link
Collaborator

@dcermak dcermak commented Jul 19, 2023

There's really no point in being a class, so let's just make it a function instead.

@schaefi
Copy link
Collaborator

schaefi commented Jul 19, 2023

Hmm, I don't see why this change is needed ? I think it's in the eye of the beholder if a namespace by class or by function is more or less pointless. However, this change introduces an API change. People who uses kiwi as API in their python program will be screwed. So where is the advantage of having this change ?

@dcermak
Copy link
Collaborator Author

dcermak commented Jul 20, 2023

I would argue that objectively the only purpose of the OCI class is the new function. It being in a class is really not adding anything here, except complexity (you now have a metaclass in there and an abstract constructor?). But I guess that's a matter of taste 🤷

@schaefi
Copy link
Collaborator

schaefi commented Jul 24, 2023

I would argue that objectively the only purpose of the OCI class is the new function. It being in a class is really not adding anything here, except complexity (you now have a metaclass in there and an abstract constructor?). But I guess that's a matter of taste shrug

It's a meta class constructing an object of another class on new(). Literally I don't see a difference in doing this in a function named new_oci() or as a class instance named OCI.new(). Well in OO design patterns you manage abstract instances of class A/B/C under a new namespace. This is a common approach and you see this all over the place in the kiwi API also for many other abstracts e.g BootLoader, VolumeManager etc etc

The thing here is that I need another justification than personal taste to change the public API interface from OCI.new() to oci_new() which is as I already said an incompatible change for which users of kiwi as python API will kill us with the question "why did you broke my implementation"

So can you give me a reason why we should create this pain ? If yes it should be done everywhere not only for OCI and it should also be done according to the deprecation policy, meaning adding @obsolete(decommission_at=... see api_helper and keeping the code around for some time before it actually leaves the planet

What looks so easy and was just a bit of a finger exercise for you can have quite some after effects

I won't do it

It could be a topic for kiwi10 ... which reminds me that I wanted to open that branch. In there we can add the changes without keeping an eye on backward compatibility. I think @Conan-Kudo also has some points on his list to do differently there

Also, add a type hint to the return type of OCI.new() so that this can now be
verified with mypy
@dcermak
Copy link
Collaborator Author

dcermak commented Jul 24, 2023

I would argue that objectively the only purpose of the OCI class is the new function. It being in a class is really not adding anything here, except complexity (you now have a metaclass in there and an abstract constructor?). But I guess that's a matter of taste

It's a meta class constructing an object of another class on new(). Literally I don't see a difference in doing this in a function named new_oci() or as a class instance named OCI.new(). Well in OO design patterns you manage abstract instances of class A/B/C under a new namespace. This is a common approach and you see this all over the place in the kiwi API also for many other abstracts e.g BootLoader, VolumeManager etc etc

The thing here is that I need another justification than personal taste to change the public API interface from OCI.new() to oci_new() which is as I already said an incompatible change for which users of kiwi as python API will kill us with the question "why did you broke my implementation"

So can you give me a reason why we should create this pain ? If yes it should be done everywhere not only for OCI and it should also be done according to the deprecation policy, meaning adding @obsolete(decommission_at=... see api_helper and keeping the code around for some time before it actually leaves the planet

What looks so easy and was just a bit of a finger exercise for you can have quite some after effects

I won't do it

I understand, let's not break the API on a matter of tastes.

I have reverted the change and would otherwise like to propose the following internal change to the function that makes it imho more obvious what happens. The API remains completely unchanged.

It could be a topic for kiwi10 ... which reminds me that I wanted to open that branch. In there we can add the changes without keeping an eye on backward compatibility. I think @Conan-Kudo also has some points on his list to do differently there

Dataclasses please 😁. Do we have a list somewhere?

@dcermak dcermak changed the title Refactor OCI.new into the function new_oci Import modules in OCI.new() explicitly Jul 24, 2023
@schaefi
Copy link
Collaborator

schaefi commented Jul 25, 2023

I have reverted the change and would otherwise like to propose the following internal change to the function that makes it imho more obvious what happens. The API remains completely unchanged.

Thanks this works for me 👍

@schaefi
Copy link
Collaborator

schaefi commented Jul 25, 2023

Dataclasses please. Do we have a list somewhere?

If you click on the "issues" tab and there on "labels" then on the label named "kiwi-10" you get a list of issues marked for kiwi-10. Feel free to add another issue and set the kiwi-10 label, this will be then our list to start with

@dcermak dcermak merged commit 3b0b60c into master Jul 25, 2023
12 checks passed
@dcermak dcermak deleted the remove-useless-class branch July 25, 2023 14:30
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.

2 participants