Skeleton structure #3
Conversation
There was a problem hiding this comment.
So, I am going to put some general thoughts/questions ideas here in the main comment, and add detail if it makes sense inline to clarify. Firstly, a question. I notice that Compose and Container locators take a locator object on init, but Git, Rpm, and 'Locator' all take distro_info. Why is this? Also, what are these two different params used for? The docstring does not explain enough for me to understand the purpose.
Related, it seems to me that the first 4 classes listed above should all extend a base Locator class. They can then take, for example **kwargs to allow the user to pass in any common params they are share, which are defined specifically in that Base class.
Lastly (I think), I notice you are using property and setter, which I am totally fine with, I actually like those, we use them in koji_wrapper as well. However, I notice you are wrapping/naming the internal variables a bit differently than we did in koji_wrapper. I think we should try to do this consistently (even if we end up deciding to do it as you have here, in which case we should update how we do it elsewhere). The first difference is the naming. In kw, we call them self.__foo vs self._foo here. The second is how we set them. I believe if you are using properties, it is recommended to always interact with them through the property accessor, and not touch them directly, as we do in the init here. (See https://github.com/release-depot/koji_wrapper/blob/master/koji_wrapper/base.py for an example of what I mean)
| @property | ||
| def git(self): | ||
| """Return object to locate a patch in git""" | ||
| if not self._git: |
There was a problem hiding this comment.
I don't think we should be doing this here, but rather in the setter. You might change the init to call the setter with the dist_info, though there may be other considerations depending how you plan to use this class. This same comment applies to other properties listed here.
| LOGGER.setLevel(logging.INFO) | ||
|
|
||
|
|
||
| class Locator(object): |
There was a problem hiding this comment.
Related to my comment about having a base Locator class with common functionality, it seems to me that this class as implemented is kind of a combination of a Base and a Factory class. If we were to move the code in that direction, we might put some of this in Base, and some in a factory.
| assert type(locator.git) is GitLocator | ||
| assert type(locator.rpm) is RpmLocator | ||
| assert type(locator.compose) is ComposeLocator | ||
| assert type(locator.container) is ContainerLocator |
There was a problem hiding this comment.
Hmm, now that I get to this point, I realize you are meaning to use 'Locator' as kind of a container for different types of locator objects. I think most of what I said is still valid, but perhaps instead of a Factory to instantiate whatever type of locator is passed in, we do want some sort of 'LocatorWrapper' (not a real name, just trying to describe what I mean). This object seems to be intended as the entry point for interacting with the various locator subclasses. It is a little hard to suggest a design here when I am not sure what has been discussed or envisioned for this process so far, but hopefully my suggestion at least give some possible directions to consider.
|
Thanks a lot for the detailed feedback and comments, I'll work to address them and also document the ideas behind some of the questions. With regard to the general structure and inconsistencies with KojiWrapper: this is nearly the exact same structure than in GitWrapper, this is where I took it from since I'm more familiar with that project. But I'll look at KojiWrapper in more details as well to see where it might make more sense to do things differently! |
|
Hey, I was just working on something else that has some similar need for properties/setters, have you considered dataclasses (https://docs.python.org/3/library/dataclasses.html)? I am trying them out in my code, and (with a few caveats) it has greatly simplified my classes and removed a lot of boilerplate code. |
|
One thing with dataclasses is you have to use python 3.7 or newer. We will want to make sure we have that in the setup.py. |
|
This needs a major rework to be consistent with the other services as well as address the concerns raised. Will abandon this PR until I have time to do this properly and probably also prepare a design doc this time to explain the context better. Thank you for the reviews! |
The skeleton structure is based on what we did for GitWrapper, since that seemed to work pretty well there. By keeping the different components in different libraries, it should be easier to extract them later if that becomes necessary/useful.
(Depends on PR#2 - I'll rebase this once the other one is ready & merged)