[GH-ISSUE #43 ] [Refactoring] Use Typer instead of argparse? #32

Author

Owner

@initialcommit-io commented on GitHub (Feb 3, 2023):

@paketb0te Hey sorry for the delay!

I'd have it store everything that is needed in the GitSimBaseCommand in there (basically replacing args - with the benefit of having type hints).

Ok cool - one thing to note is that some of the subcommand subclasses override properties from the GitSimBaseCommand. For example, the GitSimLog overrides self.numCommits and self.defaultNumCommits. Another example is GitSimRevert which overrides several other properties too.

Also, some subclasses might have their own new properties specific to that subcommand, that aren't implemented in the parent.

Maybe we even move them to a commands directory or something?

For now, lets keep them in the current directory until it gets too unruly 😄 . Since the command subclass modules are most of the files anyway, moving them to a subfolder wouldn't be that much cleaner for now, and would involve browsing into another subdir during development which is a bit annoying.

renaming, I think it would make sense to rename...

This makes sense, but I wonder if we should favor the cleaner approach of just naming the subcommand classes as Log, Status, Rebase, etc. I think in the context of the program it should be pretty easy to figure out they are scenes, and their main purpose is to identify the subcommand that is being represented.

unfortunately this change would have to be done for all commands at once

No worries! That's fine - and yes I do think we should move in this direction and implement this. We'll just need to make sure to test it thoroughly before releasing it. Appreciate all your time, input, and help with this I think it is a much cleaner and more intuitive way to organize this stuff.

@initialcommit-io commented on GitHub (Feb 3, 2023): @paketb0te Hey sorry for the delay! > I'd have it store everything that is needed in the GitSimBaseCommand in there (basically replacing args - with the benefit of having type hints). Ok cool - one thing to note is that some of the subcommand subclasses override properties from the `GitSimBaseCommand`. For example, the `GitSimLog` overrides `self.numCommits` and `self.defaultNumCommits`. Another example is `GitSimRevert` which overrides several other properties too. Also, some subclasses might have their own new properties specific to that subcommand, that aren't implemented in the parent. > Maybe we even move them to a commands directory or something? For now, lets keep them in the current directory until it gets too unruly 😄 . Since the command subclass modules are most of the files anyway, moving them to a subfolder wouldn't be that much cleaner for now, and would involve browsing into another subdir during development which is a bit annoying. > renaming, I think it would make sense to rename... This makes sense, but I wonder if we should favor the cleaner approach of just naming the subcommand classes as `Log`, `Status`, `Rebase`, etc. I think in the context of the program it should be pretty easy to figure out they are scenes, and their main purpose is to identify the subcommand that is being represented. > unfortunately this change would have to be done for all commands at once No worries! That's fine - and yes I do think we should move in this direction and implement this. We'll just need to make sure to test it thoroughly before releasing it. Appreciate all your time, input, and help with this I think it is a much cleaner and more intuitive way to organize this stuff.

kerem commented

Author

Owner

@paketb0te commented on GitHub (Feb 3, 2023):

@initialcommit-io regarding the subcommands' properties like numCommits etc, I am a bit on the fence if we should store them in a dataclass like Settings (and then reference them like Settings.commits everywhere, or if it is better to explicitly pass them as arguments to GitSimBaseCommand. I tend towards the first, do you have any preference?

(Same goes for command-specific properties)

For now, lets keep them in the current directory

Yeah makes sense 😄

naming the subcommand classes as Log, Status, Rebase, etc

Agreed, it's probably obvious enough what those are doing.

Will start working on it, but will definitely take a bit of time :)

@paketb0te commented on GitHub (Feb 3, 2023): @initialcommit-io regarding the subcommands' properties like `numCommits` etc, I am a bit on the fence if we should store them in a dataclass like `Settings` (and then reference them like `Settings.commits` everywhere, or if it is better to explicitly pass them as arguments to `GitSimBaseCommand`. I tend towards the first, do you have any preference? (Same goes for command-specific properties) > For now, lets keep them in the current directory Yeah makes sense :smile: > naming the subcommand classes as Log, Status, Rebase, etc Agreed, it's probably obvious enough what those are doing. Will start working on it, but will definitely take a bit of time :)

kerem commented

Author

Owner

@paketb0te commented on GitHub (Feb 7, 2023):

@initialcommit-io quick question: what does the self.maxrefs in GitSimStatus do?

class GitSimStatus(GitSimBaseCommand):
    def __init__(self, args: Namespace):
        super().__init__(args=args)
        self.maxrefs = 2
        self.hide_first_tag = True
        self.allow_no_commits = True

When searching the project, I see that variable set for a few of the subcommands, but never actually accessed... is that something internal to Manim? Do we need it?

@paketb0te commented on GitHub (Feb 7, 2023): @initialcommit-io quick question: what does the `self.maxrefs` in `GitSimStatus` do? ```py class GitSimStatus(GitSimBaseCommand): def __init__(self, args: Namespace): super().__init__(args=args) self.maxrefs = 2 self.hide_first_tag = True self.allow_no_commits = True ``` When searching the project, I see that variable set for a few of the subcommands, but never actually accessed... is that something internal to Manim? Do we need it?

kerem commented

Author

Owner

@initialcommit-io commented on GitHub (Feb 7, 2023):

@paketb0te I think I used that previously to set the max number of branch/tag labels that would show on top of a single commit, but I think at some point I switched to using args.max_branches_per_commit and args.max_tags_per_commit and forgot to remove the maxrefs. So yes I think we can remove that. Good catch!

@initialcommit-io commented on GitHub (Feb 7, 2023): @paketb0te I think I used that previously to set the max number of branch/tag labels that would show on top of a single commit, but I think at some point I switched to using `args.max_branches_per_commit` and `args.max_tags_per_commit` and forgot to remove the `maxrefs`. So yes I think we can remove that. Good catch!

kerem commented

Author

Owner

@paketb0te commented on GitHub (Feb 7, 2023):

Made some progress today (long train ride), only revert, rebase, merge, cherrypick are left to change, as well as some cleaning up.

@initialcommit-io FYI, I will be away for a few weeks starting february 17th, so I try to finish everything this week, hopefully you can have a look at it over the weekend so we can get this done before I leave 🤞

@paketb0te commented on GitHub (Feb 7, 2023): Made some progress today (long train ride), only `revert, rebase, merge, cherrypick` are left to change, as well as some cleaning up. @initialcommit-io FYI, I will be away for a few weeks starting february 17th, so I try to finish everything this week, hopefully you can have a look at it over the weekend so we can get this done before I leave :crossed_fingers:

kerem commented

Author

Owner

@initialcommit-io commented on GitHub (Feb 8, 2023):

@paketb0te Alright! I will do my best to take a look this weekend and merge the PR. Thanks again!

@initialcommit-io commented on GitHub (Feb 8, 2023): @paketb0te Alright! I will do my best to take a look this weekend and merge the PR. Thanks again!

kerem commented

Author

Owner

@initialcommit-io commented on GitHub (Feb 9, 2023):

@paketb0te Just fixed a minor import bug, but aside from that seems to be working well! I haven't thoroughly tested but the basic stuff seems to be working 😄 .

Any other suggestions for this one for now before we close it out?

@initialcommit-io commented on GitHub (Feb 9, 2023): @paketb0te Just fixed a minor import bug, but aside from that seems to be working well! I haven't thoroughly tested but the basic stuff seems to be working 😄 . Any other suggestions for this one for now before we close it out?

kerem commented

Author

Owner

@initialcommit-io commented on GitHub (Feb 9, 2023):

@paketb0te Hmm question about the cherrypick command/class. In Git, the command is actually cherry-pick, (with the hyphen) which is what it was using argparse. After changing to Typer, it is now set as "cherrypick" with no hyphen. Can you take a look at that?

Also it looks like the program is creating a new folder called "media/" to put the output stuff in instead of the previous "git-sim_media/". Also just want to make sure the environment variable config that we implemented still works...

@initialcommit-io commented on GitHub (Feb 9, 2023): @paketb0te Hmm question about the cherrypick command/class. In Git, the command is actually `cherry-pick`, (with the hyphen) which is what it was using argparse. After changing to Typer, it is now set as "cherrypick" with no hyphen. Can you take a look at that? Also it looks like the program is creating a new folder called "media/" to put the output stuff in instead of the previous "git-sim_media/". Also just want to make sure the environment variable config that we implemented still works...

kerem commented

Author

Owner

@initialcommit-io commented on GitHub (Feb 9, 2023):

@paketb0te Also noticed that the shorthand options like git-sim commit -m and git-sim rebase -e and git-sim -h don't work anymore. We need to add those back in.

@initialcommit-io commented on GitHub (Feb 9, 2023): @paketb0te Also noticed that the shorthand options like `git-sim commit -m` and `git-sim rebase -e` and `git-sim -h` don't work anymore. We need to add those back in.

kerem commented

Author

Owner

@paketb0te commented on GitHub (Feb 10, 2023):

question about the cherrypick command/class

Ah, I was not aware of that - we can just rename the function to cherry_pick(), underscores in function names are automatically mapped to hyphens in the commands/arguments/options names :)

Also it looks like the program is creating a new folder called "media/" to put the output stuff in instead of the previous "git-sim_media/".

I just moved the relevant code into animytions.py, I did not expect that behavior
to change 🤔

On that note, I think we can improve the readability of that part by using pathlib, which can abstract the underlying OS path so we don't need to worry about forward-slash / back-slash handling manually. I'll have a look at it.

Also just want to make sure the environment variable config that we implemented still works...

There is this awesome tool pydantic which can do that for us :D

(we can just make the Settings inherit from pydantic's BaseSettings class, then we get the whole "reading env vars into variables" for free - see Settings management)

@paketb0te commented on GitHub (Feb 10, 2023): > question about the cherrypick command/class Ah, I was not aware of that - we can just rename the function to `cherry_pick()`, underscores in function names are automatically mapped to hyphens in the commands/arguments/options names :) > Also it looks like the program is creating a new folder called "media/" to put the output stuff in instead of the previous "git-sim_media/". I just moved the relevant code into `animytions.py`, I did not expect that behavior to change :thinking: On that note, I think we can improve the readability of that part by using `pathlib`, which can abstract the underlying OS path so we don't need to worry about forward-slash / back-slash handling manually. I'll have a look at it. > Also just want to make sure the environment variable config that we implemented still works... There is this awesome tool [pydantic](https://docs.pydantic.dev/) which can do that for us :D (we can just make the `Settings` inherit from pydantic's `BaseSettings` class, then we get the whole "reading env vars into variables" for free - see [Settings management](https://docs.pydantic.dev/usage/settings/))

kerem commented

Author

Owner

@paketb0te commented on GitHub (Feb 10, 2023):

Re: the pydantic Settings, see #52 :)

@paketb0te commented on GitHub (Feb 10, 2023): Re: the pydantic Settings, see #52 :)

kerem commented

Author

Owner

@paketb0te commented on GitHub (Feb 10, 2023):

Also noticed that the shorthand options like git-sim commit -m and git-sim rebase -e and git-sim -h don't work anymore. We need to add those back in.

I'll have a look at that.

@paketb0te commented on GitHub (Feb 10, 2023): > Also noticed that the shorthand options like git-sim commit -m and git-sim rebase -e and git-sim -h don't work anymore. We need to add those back in. I'll have a look at that.

kerem commented

Author

Owner

@paketb0te commented on GitHub (Feb 10, 2023):

@initialcommit-io I only see the -e shorthand for git-sim cherry-pick, not for rebase... a typo, or did I miss something?

@paketb0te commented on GitHub (Feb 10, 2023): @initialcommit-io I only see the `-e` shorthand for `git-sim cherry-pick`, not for `rebase`... a typo, or did I miss something?

kerem commented

Author

Owner

@initialcommit-io commented on GitHub (Feb 10, 2023):

@paketb0te Sounds good, merged the 2 PR's. Yes you're right for -e shorthand I meant to say cherrypick, not rebase.

A few notes:
-It looks like the shorthand args still need to be reimplemented
-For the pydantic env variables, can you provide an example of how to set one or two of them, esp the media dir? That way I can test and add to the README
-The program is still creating a new media/ directory in the location that git-sim is run

@initialcommit-io commented on GitHub (Feb 10, 2023): @paketb0te Sounds good, merged the 2 PR's. Yes you're right for `-e` shorthand I meant to say cherrypick, not rebase. A few notes: -It looks like the shorthand args still need to be reimplemented -For the pydantic env variables, can you provide an example of how to set one or two of them, esp the media dir? That way I can test and add to the README -The program is still creating a new `media/` directory in the location that git-sim is run

kerem commented

Author

Owner

@paketb0te commented on GitHub (Feb 10, 2023):

-It looks like the shorthand args still need to be reimplemented

-> #53

For the pydantic env variables, can you provide an example of how to set one or two of them

It looks like I broke something there, it is not working as it did when playing with a minimal demo 🤔

I'l look into it

@paketb0te commented on GitHub (Feb 10, 2023): > -It looks like the shorthand args still need to be reimplemented -> #53 > For the pydantic env variables, can you provide an example of how to set one or two of them It looks like I broke something there, it is not working as it did when playing with a minimal demo :thinking: I'l look into it

kerem commented

Author

Owner

@initialcommit-io commented on GitHub (Feb 10, 2023):

@paketb0te Oh really? What command broke for you? Things seem to be working ok for me. Main bug that I see is it's still creating the media/ directory...

@initialcommit-io commented on GitHub (Feb 10, 2023): @paketb0te Oh really? What command broke for you? Things seem to be working ok for me. Main bug that I see is it's still creating the `media/` directory...

kerem commented

Author

Owner

@paketb0te commented on GitHub (Feb 10, 2023):

@initialcommit-io eh, I just had a typo, therefore my environment variables did not get picked up 😅

-> #54

Regarding the media dir issue, I have to check that out over the weekend.

@paketb0te commented on GitHub (Feb 10, 2023): @initialcommit-io eh, I just had a typo, therefore my environment variables did not get picked up :sweat_smile: -> #54 Regarding the media dir issue, I have to check that out over the weekend.

kerem commented

Author

Owner

@initialcommit-io commented on GitHub (Feb 10, 2023):

Ok great!!! Merged. Thanks for everything 😃

@initialcommit-io commented on GitHub (Feb 10, 2023): Ok great!!! Merged. Thanks for everything 😃

kerem commented

Author

Owner

@paketb0te commented on GitHub (Feb 10, 2023):

For the pydantic env variables, can you provide an example of how to set one or two of them

# Export environment variable for multiple uses
export GIT_SIM_ANIMATE=true
git-sim log
git-sim commit

# Use environment variable for a single command
GIT_SIM_ANIMATE=true git-sim log

I think pydantic can even read from .env files, but I am not 100% sure.

Edit: Yes, it does: Dotenv (.env) support

@paketb0te commented on GitHub (Feb 10, 2023): > For the pydantic env variables, can you provide an example of how to set one or two of them ```bash # Export environment variable for multiple uses export GIT_SIM_ANIMATE=true git-sim log git-sim commit # Use environment variable for a single command GIT_SIM_ANIMATE=true git-sim log ``` I *think* pydantic can even read from `.env` files, but I am not 100% sure. Edit: Yes, it does: [Dotenv (.env) support](https://docs.pydantic.dev/usage/settings/#dotenv-env-support)

kerem commented

Author

Owner

@initialcommit-io commented on GitHub (Feb 10, 2023):

@paketb0te Awesome!! That's a really cool and convenient feature that it allows configuring any of the settings as env variables like that!

FYI I was playing with the media_dir issue and it's kinda weird. It seems the manim config options are not being set for some reason, such as config.media_dir, config.background_color, etc.

So it's not just the media_dir, it's anything that gets set like config.xyz. You can confirm this by trying to run git-sim --light-mode log, and notice the background color stays black.

It's acting like something in the new setup is causing the config options to be set on a different instance of the manim config object, i.e. not the global one for some reason. Here is a link to the manim config docs: https://docs.manim.community/en/stable/guides/configuration.html#the-manimconfig-class

Can you take a look at that to try and make sure the global config class is the one we are setting the properties on? I think that should fix these issues...

One other thing - when @abhijitnathwani added the original environment variable git_sim_media_dir, he appended the repo_name to the supplied path (the logic to build the repo name string is still in there, but no longer used) to compartmentalize the stuff for different repos, so it's like: git_sim_media_dir + "git-sim_media" + repo_name

That should only happen if the environment variable value is set and the command line --media_dir option is default, so I think we might need to add back some of @abhijitnathwani 's code for that, it was this:

# If the env variable is set and no argument provided, use the env variable value
-    if os.getenv("git_sim_media_dir") and Settings.media_dir == ".":
-        config.media_dir = os.path.join(
-            os.path.expanduser(os.getenv("git_sim_media_dir")),
-            "git-sim_media",
-            repo_name,
-        )

@initialcommit-io commented on GitHub (Feb 10, 2023): @paketb0te Awesome!! That's a really cool and convenient feature that it allows configuring any of the settings as env variables like that! FYI I was playing with the media_dir issue and it's kinda weird. It seems the manim config options are not being set for some reason, such as `config.media_dir`, `config.background_color`, etc. So it's not just the media_dir, it's anything that gets set like `config.xyz`. You can confirm this by trying to run `git-sim --light-mode log`, and notice the background color stays black. It's acting like something in the new setup is causing the config options to be set on a different instance of the manim config object, i.e. not the global one for some reason. Here is a link to the manim config docs: https://docs.manim.community/en/stable/guides/configuration.html#the-manimconfig-class Can you take a look at that to try and make sure the global config class is the one we are setting the properties on? I think that should fix these issues... --- One other thing - when @abhijitnathwani added the original environment variable `git_sim_media_dir`, he appended the repo_name to the supplied path (the logic to build the repo name string is still in there, but no longer used) to compartmentalize the stuff for different repos, so it's like: `git_sim_media_dir + "git-sim_media" + repo_name` That should only happen if the environment variable value is set and the command line --media_dir option is default, so I think we might need to add back some of @abhijitnathwani 's code for that, it was this: ```python # If the env variable is set and no argument provided, use the env variable value - if os.getenv("git_sim_media_dir") and Settings.media_dir == ".": - config.media_dir = os.path.join( - os.path.expanduser(os.getenv("git_sim_media_dir")), - "git-sim_media", - repo_name, - ) ```

kerem commented

Author

Owner

@abhijitnathwani commented on GitHub (Feb 11, 2023):

Hi @paketb0te @initialcommit-io ,
These are some BIG changes in the whole flow of the application. I need to re-map the application flow and understand how to use Typer. It looks intuitive, however, need to go through the docs to understand the usage.

FYI, these changes don't work on certain python 3 versions. I tried pulling the latest changes and it stopped working. I tested with python 3.8.6 and python 3.9.0. I was met with the following error:

    File "D:\git-sim\git_sim\settings.py", line 22, in Settings
    files: list[pathlib.Path] | None = None
TypeError: 'type' object is not subscriptable

I upgraded to python 3.11.2, the latest available, and it started working. These changes may be breaking for some users as they may not always use the latest versions. I suggest we make it compatible with at least upto Python 3.8. If not, we need to make changes to README, dockerfile and some graceful message asking user to update their python version to run git-sim, which imho, doesn't make sense.

@abhijitnathwani commented on GitHub (Feb 11, 2023): Hi @paketb0te @initialcommit-io , These are some BIG changes in the whole flow of the application. I need to re-map the application flow and understand how to use `Typer`. It looks intuitive, however, need to go through the docs to understand the usage. FYI, these changes don't work on certain python 3 versions. I tried pulling the latest changes and it stopped working. I tested with python 3.8.6 and python 3.9.0. I was met with the following error: ``` File "D:\git-sim\git_sim\settings.py", line 22, in Settings files: list[pathlib.Path] | None = None TypeError: 'type' object is not subscriptable ``` I upgraded to python 3.11.2, the latest available, and it started working. These changes may be breaking for some users as they may not always use the latest versions. I suggest we make it compatible with at least upto Python 3.8. If not, we need to make changes to README, dockerfile and some graceful message asking user to update their python version to run `git-sim`, which imho, doesn't make sense.

kerem commented

Author

Owner

@initialcommit-io commented on GitHub (Feb 11, 2023):

@abhijitnathwani Yes the refactoring with Typer and Pydantic does restructure the code nicely, although we are still hammering out some kinks as you can tell.

Great catch on the Python 3 compatibility issues - I have been testing in 3.11.1 so would have missed those breaking changes for the earlier versions (which also speaks to needing a good test plan like we've been discussing and now have the other thread for). I will wait to push a new release to PyPI until we have everything sorted out and validated in various Python versions.

I agree with the need to support the earlier Python versions, upon release the tool supported all the way back to 3.7, so that would be ideal.

@initialcommit-io commented on GitHub (Feb 11, 2023): @abhijitnathwani Yes the refactoring with Typer and Pydantic does restructure the code nicely, although we are still hammering out some kinks as you can tell. Great catch on the Python 3 compatibility issues - I have been testing in 3.11.1 so would have missed those breaking changes for the earlier versions (which also speaks to needing a good test plan like we've been discussing and now have the other thread for). I will wait to push a new release to PyPI until we have everything sorted out and validated in various Python versions. I agree with the need to support the earlier Python versions, upon release the tool supported all the way back to 3.7, so that would be ideal.

kerem commented

Author

Owner

@initialcommit-io commented on GitHub (Feb 12, 2023):

@paketb0te I was working on this and moving the Manim config stuff back into the main() function is the only place where it seems to get set properly. I'm not sure exactly why that happens, but I am going to refactor it back over there so we can keep moving forward. I will make that commit soon.

@initialcommit-io commented on GitHub (Feb 12, 2023): @paketb0te I was working on this and moving the Manim config stuff back into the `main()` function is the only place where it seems to get set properly. I'm not sure exactly why that happens, but I am going to refactor it back over there so we can keep moving forward. I will make that commit soon.

kerem commented

Author

Owner

@paketb0te commented on GitHub (Feb 12, 2023):

@paketb0te I was working on this and moving the Manim config stuff back into the main() function is the only place where it seems to get set properly. I'm not sure exactly why that happens, but I am going to refactor it back over there so we can keep moving forward. I will make that commit soon.

Hm, when inspecting the Manim config, I actually see that the values get passed in correctly into manims's config object, but when I look at scene.renderer.file_writer.movie_file_path, it uses the media dir, instead of git-sim_media 🤔

I think it should not make a difference whether the animation logic is called from main() or from anywhere else, so I am trying to understand what the actual problem is we are facing, and what is just a symptom resulting from it...

@paketb0te commented on GitHub (Feb 12, 2023): > @paketb0te I was working on this and moving the Manim config stuff back into the `main()` function is the only place where it seems to get set properly. I'm not sure exactly why that happens, but I am going to refactor it back over there so we can keep moving forward. I will make that commit soon. Hm, when inspecting the Manim config, I actually see that the values get passed in correctly into manims's config object, but when I look at `scene.renderer.file_writer.movie_file_path`, it uses the `media` dir, instead of `git-sim_media` :thinking: I think it *should* not make a difference whether the animation logic is called from `main()` or from anywhere else, so I am trying to understand what the actual problem is we are facing, and what is just a symptom resulting from it...

kerem commented

Author

Owner

@initialcommit-io commented on GitHub (Feb 13, 2023):

@paketb0te I moved the config stuff back into main along with a few minor updates to get it all working again. It's only about 20 lines of code so it doesn't clutter up main() too much. From my testing it seemed like there were multiple instances of the manim config or something, so setting the config values in another module seemed to not be affecting the correct instance. Still not totally sure tho, but it works fine after moving into the main() method.

@initialcommit-io commented on GitHub (Feb 13, 2023): @paketb0te I moved the config stuff back into main along with a few minor updates to get it all working again. It's only about 20 lines of code so it doesn't clutter up main() too much. From my testing it seemed like there were multiple instances of the manim config or something, so setting the config values in another module seemed to not be affecting the correct instance. Still not totally sure tho, but it works fine after moving into the main() method.

kerem commented