FeedTheCube / CogDoc

0 stars 0 forks source link

Removing the package heirarchy and unifying all classes #11

Closed hyndgrinder closed 6 years ago

hyndgrinder commented 6 years ago

I took a stab at bringing the classes together under the classes folder. I also created an empty init.py file. I was having trouble with my imports and subsequent class calls, and tried a bunch of things to fix it. This combination of import statements and init.py seems to have solved the problem with the module not callable errors I was receiving. However, I may not be using efficient code.

@SinclairC Please review and commit if acceptable.

hyndgrinder commented 6 years ago

to close #10

SinclairC commented 6 years ago

It seems I should clarify things. I absolutely do not think we should unify all classes, just that we should remove the methods that are used as "static" methods from the classes and unify those. It's very useful to have a Query class, DataItem class, etc. and I don't think those should be removed.

Just in case there's a communication issue here:

"static" == "something that is accessed without instantiating an object" "methods" ~= "functions" (I'm not actually sure which term is normally used in Python)

SinclairC commented 6 years ago

Did you get a chance to check out my SincTests branch? Apart from there still being too many folders, it's mostly what I'm recommending.

hyndgrinder commented 6 years ago

It seems I should clarify things. I absolutely do not think we should unify all classes, just that we should remove the methods that are used as "static" methods from the classes and unify those. It's very useful to have a Query class, DataItem class, etc. and I don't think those should be removed.

Just in case there's a communication issue here:

"static" == "something that is accessed without instantiating an object" "methods" ~= "functions" (I'm not actually sure which term is normally used in Python)

Yes, I understood your suggestion, and the poor communication is coming from my side of the conversation. I especially like the simplified explanation of static and methods. This drives home the point. However, what are @classmethods & @staticmethods? And doesn't it make a certain amount of sense to keep code that services the class in the class? I feel like this conversation is more about project organization than it is about functionality or performance... The output of the lesson I took which explained the difference between them is here. In it the instructor seems to make anything that looks-up or creates an instance of the object is decorated with @classmethod. Anything that is just changing the values of the class without instantiating a new instance is an @staticmethod. This seems to fall in line with what you are saying. However, there appear to be some shortcuts available from @classmethod like cls(**classname) which saves you having to type out all of the parameters of the class. I'm not sure that's still available if we move it to the Util class, is it?

In an attempt to understand the difference, I've branched this PR again and am going to do some research so I can better understand what your perspective. I like the idea of keeping a class' methods with the class; it organizes better in my head, but I realize that it is a purely subjective reasoning. I'm afraid there are technical reasons putting all the methods on the Util class that I'm still not understanding. I see the Util class being more about common functions, like making connections to the database, or opening/closing files, or things of that ilk...

Did you get a chance to check out my SincTests branch? Apart from there still being too many folders, it's mostly what I'm recommending.

Yes, in Pull Request #11 (this PR) I have removed the all of class packages (a folder with an init.py file inside), and moved the classes over to the Classes folder. I also added an init.py to the Classes folder (which I believe turns the directory into a package).

So, I think you just need to clone this PR, confirm that it's structured according to your expectations, and merge the PR back to your initial commit branch if it's acceptable. Also, at that point we'll probably want to do a Pull Request on on your SincTests branch to get it back onto CogDoc init_commit branch.

SinclairC commented 6 years ago

And doesn't it make a certain amount of sense to keep code that services the class in the class? I feel like this conversation is more about project organization than it is about functionality or performance...

Well, organization is relatively difficult to change once there's a lot of code in place so it's something to think about early on. The way things are set up will affect everything that comes later. Additionally, if we bury methods in various classes we may end up losing track of what methods exist once we have more of the code in place.

I agree that putting methods in classes when they serve the class can make sense (although I'd personally put things that are never used by the objects of that class outside of it). My question, I guess, is: Does the "getQueries()" method really service the Query class? The Query objects may never use it. I could see something like "getDataItems()" serving the Query class, though, because Query objects might end up having things that work with those items and therefore need to grab them all. That said, since there may be situations where we want DataItems extracted from different collections or something, Utils.getDataItems() makes it clearer that getDataItems can freely be used apart from the Query class.

If you do want to keep your structure, then I'll need to figure out where the lines are. Does anything that grabs a Query object belong in the Query class, even if it has more to do with something else? Utils makes that sort of thing clearer.

However, what are @classmethods & @staticmethods?

As I understand it, @classmethod is for static methods in a class that can only affect the class itself or an object of the class that is passed to the method in an argument. @staticmethod is just for normal static methods that won't be called internally by an object. If you want to keep things using your structure, then we'll likely be using @staticmethod in a lot of places.

Anyway, I think I've said all I wanted to about the class/method organization. If you still find your old structure easier to understand for classes/methods and want to keep it, I'll work with it.

I'll hopefully get a chance to grab the current state of the repo at some point later today. From a quick glance at it on here, it looks like I'd expect it to. Good call on the init.py. I completely forgot about that.

hyndgrinder commented 6 years ago

Ok, I think I see the importance of the difference now. It's a subtle point, but it is an important point because, like you said, going from here we would not be wanting to be change back and forth. And BTW, its not my structure per se, it was just how I learned. So the questions are not questioning your skill or choices, but to just bring me to equilibrium as to why people would take one approach or the other. But I definitely appreciate you taking the time to explain. Thx.

Yes, this pull is set up as you'd imagined, I think. There are classes in Classes folder, there is a Util class with the static methods, and I think it's ready to become the jumping off point for a flurry of build activity, if we can use this as our standard