[GH-ISSUE #38] [Refactoring] Change inheritance structure?

kerem commented

2026-03-02 16:47:17 +03:00

Owner

Originally created by @paketb0te on GitHub (Jan 27, 2023).
Original GitHub issue: https://github.com/initialcommit-com/git-sim/issues/38

@initialcommit-io Following our discussion in #8, I have created an example of what I think would be a simpler way of handling class inheritance - see my branch inheritance (only git-sim log for now).

Basically I did the following:

make GitSimBaseCommand a Manim MovingCameraScene
replace all occurences of self.scene in GitSimBaseCommand with self.
GitSimBaseCommand takes args instead of scene as argument to its __init__()
accordingly, GitSimLog passes args instead of scene to its super().__init__()
use a function instead of the GitSim class to select the correct subclass for each command

Not sure if that makes sense, but at least my brain can process it better like this 😆

Using Typer to make the whole argument- and option-handling easier could be a next step, but I think this would be a great starting point, if you are interested / if it makes sense to you.

Obviously there is no point in changing that up only so that I understand it better, so I'd highly appreciate any feedback!

Originally created by @paketb0te on GitHub (Jan 27, 2023). Original GitHub issue: https://github.com/initialcommit-com/git-sim/issues/38 @initialcommit-io Following our discussion in #8, I have created an example of what I think would be a simpler way of handling class inheritance - see my branch [inheritance](https://github.com/paketb0te/git-sim/tree/inheritance/git_sim) (only `git-sim log` for now). Basically I did the following: - make `GitSimBaseCommand` a Manim `MovingCameraScene` - replace all occurences of `self.scene` in `GitSimBaseCommand` with `self.` - `GitSimBaseCommand` takes `args` instead of `scene` as argument to its `__init__()` - accordingly, `GitSimLog` passes `args` instead of `scene` to its `super().__init__()` - use a function instead of the `GitSim` class to select the correct subclass for each command Not sure if that makes sense, but at least _my_ brain can process it better like this :laughing: Using Typer to make the whole argument- and option-handling easier could be a next step, but I think this would be a great starting point, if you are interested / if it makes sense to you. Obviously there is no point in changing that up only so that _I_ understand it better, so I'd highly appreciate any feedback!

kerem closed this issue

2026-03-02 16:47:17 +03:00

kerem commented

2026-03-02 16:47:18 +03:00

Author

Owner

@initialcommit-io commented on GitHub (Jan 27, 2023):

Great summary @paketb0te !

After reviewing I do like this better than the current setup for now, it's clearer and more straightforward - great suggestion! Although I do like the idea of separation of Manim scene from the GitSimCommand, I think in essence a scene can be thought of as an inherent part of a Git-Sim simulation/command, so it can make sense to combine it and think about it that way.

Can you take care of the refactor for this in a new PR?

Random thought: For the forseeable future I do see the project relying on Manim, but I do wonder if down the line it might be useful to keep our options open for plugging in other animation libraries, and even maybe making that a customizable thing. We could potentially do this by abstracting out the animation library, and in that kind of setup it might be worth thinking about a way to separate the actual "drawing and animating" stuff from the "git command stuff". I think that is sort of what I was going for with my current design, but obviously it's still pretty tightly coupled...

Do you have any thoughts on the design for keeping our options open/flexible to something like this down the line?

@initialcommit-io commented on GitHub (Jan 27, 2023): Great summary @paketb0te ! After reviewing I do like this better than the current setup for now, it's clearer and more straightforward - great suggestion! Although I do like the idea of separation of Manim scene from the GitSimCommand, I think in essence a scene can be thought of as an inherent part of a Git-Sim simulation/command, so it can make sense to combine it and think about it that way. Can you take care of the refactor for this in a new PR? Random thought: For the forseeable future I do see the project relying on Manim, but I do wonder if down the line it might be useful to keep our options open for plugging in other animation libraries, and even maybe making that a customizable thing. We could potentially do this by abstracting out the animation library, and in that kind of setup it might be worth thinking about a way to separate the actual "drawing and animating" stuff from the "git command stuff". I think that is sort of what I was going for with my current design, but obviously it's still pretty tightly coupled... Do you have any thoughts on the design for keeping our options open/flexible to something like this down the line?

kerem commented

2026-03-02 16:47:18 +03:00

Author

Owner

@paketb0te commented on GitHub (Jan 28, 2023):

Glad to hear you like it!

Actually I just realised that maybe I was a bit over-eager...

What confused me was that we set self.args = args in GitSim's __init__(), and then access the args and everything contained ther as self.scene.args.foo in the GitSimBaseCommand class.

Maybe GitSimBaseCommand can initialize a new MovingCameraScene instead of being passed one - the only thing happening before is the setting of the args ins GitSim:

class GitSim(m.MovingCameraScene):
    def __init__(self, args):
        super().__init__()
        self.args = args

        if self.args.light_mode:
            self.fontColor = m.BLACK
        else:
            self.fontColor = m.WHITE
    ...


class GitSimBaseCommand:
    ...
    def show_intro(self):
        if self.scene.args.animate and self.scene.args.show_intro:
            self.scene.add(self.logo)
            ...

So, maybe we just need to separate the args from the scene? 🤔

class GitSimBaseCommand:
    def __init__(self, args):
        self.scene = m.MovingCameraScenene
        ...
    def show_intro(self):
        if self.args.animate and self.args.show_intro:
            self.scene.add(self.logo)
            ...

And to select the right subclass, we could still use get_scene_for_command() as in my example:

def get_scene_for_command(args: Namespace) -> GitSimBaseCommand:

    scene = None

    if args.subcommand == "log":
        scene = GitSimLog(args)
    elif args.subcommand == "status":
        scene = GitSimStatus(args)
    ...
    return scene

Thoughts?

@paketb0te commented on GitHub (Jan 28, 2023): Glad to hear you like it! Actually I just realised that _maybe_ I was a bit over-eager... What confused me was that we set `self.args = args` in `GitSim`'s `__init__()`, and then access the args and everything contained ther as `self.scene.args.foo` in the `GitSimBaseCommand` class. Maybe `GitSimBaseCommand` can initialize a new `MovingCameraScene` instead of being passed one - the only thing happening before is the setting of the args ins `GitSim`: ```python class GitSim(m.MovingCameraScene): def __init__(self, args): super().__init__() self.args = args if self.args.light_mode: self.fontColor = m.BLACK else: self.fontColor = m.WHITE ... class GitSimBaseCommand: ... def show_intro(self): if self.scene.args.animate and self.scene.args.show_intro: self.scene.add(self.logo) ... ``` So, maybe we just need to separate the `args` from the `scene`? :thinking: ```python class GitSimBaseCommand: def __init__(self, args): self.scene = m.MovingCameraScenene ... def show_intro(self): if self.args.animate and self.args.show_intro: self.scene.add(self.logo) ... ``` And to select the right subclass, we could still use `get_scene_for_command()` as in my example: ```python def get_scene_for_command(args: Namespace) -> GitSimBaseCommand: scene = None if args.subcommand == "log": scene = GitSimLog(args) elif args.subcommand == "status": scene = GitSimStatus(args) ... return scene ``` Thoughts?

kerem commented

2026-03-02 16:47:18 +03:00

Author

Owner

@initialcommit-io commented on GitHub (Jan 28, 2023):

@paketb0te Hmm so yeah this sort of brings up something I forgot to mention before - that the GitSim class isn't just an "empty" class - it's the scene that has its construct() method called by Manim. As far as I know, all of the animation stuff that goes into a scene needs to be called from within a construct() method, so you can think of the GitSim class as kind of a wrapper class for that, and to choose which subcommand functionality to actually run based on args, then finally feed in the scene object and args into the GitSimCommand flow.

The main reason I made GitSim a class and not a function is that it needs to have the construct() method in order to kick off Manim's scene generation process.

If GitSimBaseCommand inherits the MovingCameraScene then its own construct() method would need to be called, and it just seemed weird for a Git command class to have all these manim members as a part of it.

In the method you proposed above, where does the construct() live that gets called?

@initialcommit-io commented on GitHub (Jan 28, 2023): @paketb0te Hmm so yeah this sort of brings up something I forgot to mention before - that the GitSim class isn't just an "empty" class - it's the scene that has its `construct()` method called by Manim. As far as I know, all of the animation stuff that goes into a scene needs to be called from within a construct() method, so you can think of the GitSim class as kind of a wrapper class for that, and to choose which subcommand functionality to actually run based on `args`, then finally feed in the scene object and args into the GitSimCommand flow. The main reason I made GitSim a class and not a function is that it needs to have the `construct()` method in order to kick off Manim's scene generation process. If GitSimBaseCommand inherits the MovingCameraScene then its own construct() method would need to be called, and it just seemed weird for a Git command class to have all these manim members as a part of it. In the method you proposed above, where does the `construct()` live that gets called?

kerem commented

2026-03-02 16:47:18 +03:00

Author

Owner

@paketb0te commented on GitHub (Jan 28, 2023):

Ah, I see.

If we need a subclass of the Manim Scene, I guess the approach to make GitSimBaseCommand a subclass of m.MovingCameraScene (and then subclassing the individual GitSimFoo classes for each command from that) works better.

@paketb0te commented on GitHub (Jan 28, 2023): Ah, I see. If we _need_ a subclass of the Manim Scene, I guess the approach to make `GitSimBaseCommand` a subclass of `m.MovingCameraScene` (and then subclassing the individual `GitSimFoo` classes for each command from that) works better.

kerem commented

2026-03-02 16:47:19 +03:00

Author

Owner

@paketb0te commented on GitHub (Jan 28, 2023):

@initialcommit-io just to make sure we are on the same page: We go ahead with the "original" idea of this issue as described in my first comment (GitSimBaseCommand inherits from m.MovingCameraScene, and then the class for each command inherits from that)?

@paketb0te commented on GitHub (Jan 28, 2023): @initialcommit-io just to make sure we are on the same page: We go ahead with the "original" idea of this issue as described in my [first comment](https://github.com/initialcommit-com/git-sim/issues/38#issue-1560436607) (`GitSimBaseCommand` inherits from `m.MovingCameraScene`, and then the class for each command inherits from that)?

kerem commented

2026-03-02 16:47:19 +03:00

Author

Owner

@initialcommit-io commented on GitHub (Jan 28, 2023):

@paketb0te Yeah let's try it! So I guess you will be adding a construct() method into the GitSimBaseCommand which just calls the execute() method.

Then I think we can get rid of the git_sim.py entirely and move the command selection if statement into __main__.py, which would just directly instantiate the command class and pass in the args.

Let me know if I'm missing anything...

@initialcommit-io commented on GitHub (Jan 28, 2023): @paketb0te Yeah let's try it! So I guess you will be adding a `construct()` method into the GitSimBaseCommand which just calls the `execute()` method. Then I think we can get rid of the `git_sim.py` entirely and move the command selection `if` statement into `__main__.py`, which would just directly instantiate the command class and pass in the args. Let me know if I'm missing anything...

kerem commented

2026-03-02 16:47:19 +03:00

Author

Owner

@paketb0te commented on GitHub (Jan 28, 2023):

Yeah basically that.

If I understand the docs for the Scene class correctly, we only need to override the construct() method (which contains all the animation logic) and could skip the execute() method?

E.g.

class GitSimBaseCommand:
    # all the methods that are not specific to a command go here,
    # e.g. show_intro(), show_outro(), ...
    ...


class GitSimLog(GitSimBaseCOmmand):
    def construct(self):
        # this contains the actual animation logic for this particular command,
        self.show_intro()
        ...
        self.show_outro()

Since that could be a bit confusing with the passed args, maybe we can create a dataclass that stores all the options / flags that are gathered from argparse?

(We could then later change that to be a pydantic BaseSettings class (which automatically parses env vars with the same name as the specified attributes) 😁

I'll see what I can do the next few days.

@paketb0te commented on GitHub (Jan 28, 2023): Yeah basically that. If I understand the docs for the [`Scene`](https://docs.manim.community/en/stable/reference/manim.scene.scene.Scene.html) class correctly, we only need to override the `construct()` method (which contains all the animation logic) and could skip the `execute()` method? E.g. ```python class GitSimBaseCommand: # all the methods that are not specific to a command go here, # e.g. show_intro(), show_outro(), ... ... class GitSimLog(GitSimBaseCOmmand): def construct(self): # this contains the actual animation logic for this particular command, self.show_intro() ... self.show_outro() ``` Since that could be a bit confusing with the passed `args`, maybe we can create a `dataclass` that stores all the options / flags that are gathered from argparse? (We could then later change that to be a [pydantic](https://docs.pydantic.dev/) [`BaseSettings`](https://docs.pydantic.dev/usage/settings/) class (which automatically parses env vars with the same name as the specified attributes) :grin: I'll see what I can do the next few days.

kerem commented

2026-03-02 16:47:19 +03:00

Author

Owner

@initialcommit-io commented on GitHub (Jan 28, 2023):

@paketb0te Yes I guess we only really do need construct(). In that case, you'll just be replacing the execute() methods (which aren't Manim methods - it's just a custom method I made up to make a call from the original GitSim class) with construct().

And yes I think getting args into the mix is our last remaining puzzle piece at the moment. It is a little messy to have to pass the args into the Git command class when it overrides init from the GitSimBaseCommand class, not sure if there is a cleaner way. I'm open to either of the options you suggested if you want to take a crack at it we can review when ready.

Thanks again!!!

@initialcommit-io commented on GitHub (Jan 28, 2023): @paketb0te Yes I guess we only really do need `construct()`. In that case, you'll just be replacing the `execute()` methods (which aren't Manim methods - it's just a custom method I made up to make a call from the original `GitSim` class) with `construct()`. And yes I think getting args into the mix is our last remaining puzzle piece at the moment. It is a little messy to have to pass the args into the Git command class when it overrides __init__ from the GitSimBaseCommand class, not sure if there is a cleaner way. I'm open to either of the options you suggested if you want to take a crack at it we can review when ready. Thanks again!!!

kerem commented

2026-03-02 16:47:19 +03:00

Author

Owner

@initialcommit-io commented on GitHub (Jan 29, 2023):

Thanks for this! Changes merged.

@initialcommit-io commented on GitHub (Jan 29, 2023): Thanks for this! Changes merged.

Rows
Columns

[GH-ISSUE #38] [Refactoring] Change inheritance structure? #28