mirror of
https://github.com/initialcommit-com/git-sim.git
synced 2026-04-27 03:25:53 +03:00
[GH-ISSUE #8] Clean up star imports #7
Labels
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
starred/git-sim#7
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @paketb0te on GitHub (Jan 23, 2023).
Original GitHub issue: https://github.com/initialcommit-com/git-sim/issues/8
Hi @initialcommit-io,
This is a really cool project and I#d like to contribute where I can!
One thing that struck me while looking at the code was that there seem to be quite a lot of "star imports" (
from foo import *), which are considered bad practice AFAIK.They also make it much more difficult to infer what the exact types of the different objects are (since you can't have any type hints).
Would you mind if I clean those up a little bit and replace them with explicit imports?
@paketb0te commented on GitHub (Jan 23, 2023):
@initialcommit-io could you help me understand what the
sceneparameter passed to GitSimBaseCommand's__init__()function is?I think it is a
GitSimobject (since all instances are passedselfon creation (in theconstruct()function), and theGitSimclass has matching attributes / functions, but then again I don't understand what would be the point of creating an extra argument when it is always just passedself.I am a bit confused 😅
@paketb0te commented on GitHub (Jan 23, 2023):
WIP -> branch clean_star_imports on my fork
@initialcommit-io commented on GitHub (Jan 24, 2023):
Thanks for this! I like what you did with
git_sim.py, it is nice and clean in there. However, for thegit_sim_base_command.pyI don't like that we have to prefix every Manim thing withmanim.I also don't really like adding an individual import for every single Manim thing (what is this, Java?! :p), so I think maybe the star import is ok to keep there... And in other files that will utilize an arbitrary number of Manim objects. Thoughts? Are there any other options you can think of for a scenario like this?And as for the
sceneparameter... First in addition to the GitSimBaseCommand's__init__(), it also gets passed to all the other command class__init__()methods (such as GitSimReset, GitSimAdd, etc, since they override the init() from the base class).And yes you're right it expects to be passed in a GitSim object, which inherits from MovingCameraScene from Manim. So yes, GitSim is actually a scene object that gets created in
__main__.pyon line 88:As you can see the command line arguments
argsget passed in, and then GitSim makes that a local attribute to be passed into the individual command object that is being run. This gives each object the ability to access data from the scene itself fromself.scenewhich gets set asself.scene = scenein the base command and individual command inits.The main reason for this is so that Manim scene actions can be taken within each of the base command class and individual command classes, such as
self.scene.play(...), or more generallyself.scene.anything....Note that since the arguments were added directly to the scene too (maybe better to separate those honestly...), I currently access those like
self.scene.args.commits, for example.Let me know if that makes sense. In a nutshell the reason is so that the individual commands can get access to Manim scene methods, attributes, and functionality.
@paketb0te commented on GitHub (Jan 24, 2023):
I understand that we do not want to explicitly import every used object from the
manimbase package (I started with that, but quickly realized that it would be a looong import list...).As for "not prefixing every Manim thing wirh
manim": Wouldimport manim as mbe an acceptable middle-ground?Easy/quick to type, but we get all the advantages of not polluting our modules namespace with a star import 😁
the explicit import makes it much easier to see where an object is defined, and it helps against accidentally overriding an object (although I think the main advantage in our case would be improved readability). Also helps with static code analysis / type hints.
Let me know what you think :)
@paketb0te commented on GitHub (Jan 24, 2023):
@initialcommit-io thanks for the explanation re: the scene objects.
I think we can improve the class relationships there, I just ran into a circular import when trying to add type hints...
Could the
GitSimBaseCommandinherit from Manim'sMovingCameraScenedirectly?Right now the
GitSimclass itself does nothing, it is just an intermediary to instantiate the appropriate class and store the args - both of which could be moved out.@initialcommit-io commented on GitHub (Jan 24, 2023):
Great points and suggestions. Ok - you sold me on the star imports. Let's use
import manim as mas you suggested. I agree with your points about easy to type, and not polluting the namespace. Let's incorporate that in each of the files that imports Manim. It would be great if you can submit a pull request this and I will review/accept.For the GitSim class, the main reason I set it up this way is that I was thinking of each Git command class (like GitSimLog or GitSimReset) as representing a Git command itself wit corresponding Git functionality, not representing a Manim scene.
Inheriting
MovingCameraScenein each Git command class (or even just in GitSimBaseCommand) does seem tempting for the reasons you mentioned, but would imply that each Git command class is also its own scene. Maybe that's OK since this tool is created to create scenes for Git commands, as opposed to Git commands themselves?But I kind of like the design of having 1 overarching scene object, which the Git command classes can simply modify/add-to as needed. That's why I passed the scene instance in as an argument to the init...
Thoughts? Can you think of any other way to preserve separate scene and Git command classes that would avoid the circular references you mentioned?
@paketb0te commented on GitHub (Jan 25, 2023):
Hi @initialcommit-io , I opened #17 for this issue.
Regarding the class inheritance, I'll try to think of something, but I guess we can continue that discussion in a separate thread as it is not directly related to the star-imports 😃
@paketb0te commented on GitHub (Jan 25, 2023):
@initialcommit-io I realize I am having difficulties wrapping my head around the nested classes and their (partially circular) references 🤪
Recently I stumbled upon Typer, which probably can simplify the whole command / subcommand / arguments stuff, but I experimented a bit with it and it would definitely require heavy changes (although I personally find the result much easier to reason about).
@initialcommit-io commented on GitHub (Jan 25, 2023):
@paketb0te Hmm can you identify more specifically what you mean by "partially circular references"? The only one I can think of is what we talked about before where the GitSim class passes itself into each Git subcommand instance, like
self.command = GitSimLog(self). The reason I did this was to maintain separation between GitSimCommand classes and the Scene itself. It actually makes for quite a clean setup imo.Here is how I think about it:
Program entry point: main.py
git_sim.py (contains the GitSim.py scene subclass)
.contruct()to instantiate the appropriate Git subcommand and then.execute()itgit_sim_base_command:
Contains common Git-Sim command attributes and methods to be shared by subclass commands, so basically all current and future Git subcommand classes (like GitSimStatus) will inherit from GitSimBaseCommand. These inherited methods can be (and are) overridden where necessary.
git_sim_xyz:
One of these is defined for each subcommand supported by Git-Sim. In general these will override the init and
execute()methods of the BaseCommand class, which specify which command-specific actions to take.Finishing up:
Once the code in the corresponding execute() method has finished running, flow goes back to the main.py for some final logic based on whether an image or video should be generated.
One of the main reasons I did it this way was that originally I had all the code for each Git subcommand in it's own module, but that got to be very unruly when the number of commands grows, because every module ends up with minor changes and differences in their code, and sometimes a single change needs to be propagated to ALL files which is a nightmare for maintenance.
The current setup allows most global changes to be done to the GitSimBaseCommand class, with the exception of changes to specific methods which have been overridden by a GitSimCommand class.
So the main way I reason about the code when I need to figure out where something happens, is I open the corresponding file for the command I am working on, like GitSimAdd for "git add", then look straight at the
execute()method. That will tell you exactly what actions are happening. From there you might either need to look into the GitSimBaseCommand to find the inherited method, or just look lower down the same module if the method was overridden.BUT, I am curious what kind of other structures might be appropriate, while taking into account some of my design preferences regarding separation of scene and command, and also using inheritance to encapsulate the functionality for methods that are common to all (or many) commands.
Can you provide a sample structure of how Typer would adjust the structure of this codebase?
@paketb0te commented on GitHub (Jan 26, 2023):
Yeah I was referring to that - it's not really a circular reference I guess, bad naming on my part.
I'll try to hack something together to show what I think could be useful with Typer, might take a while though because I don't have much time the next days...
@initialcommit-io commented on GitHub (Jan 26, 2023):
Cool, looking forward to seeing it! I do think that if we decide to do a structural refactoring of the codebase we should do it earlier as opposed to later, while the codebase is still pretty small.