Comments (29)
@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!
from git-sim.
Ok great!!! Merged. Thanks for everything 😃
from git-sim.
@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.
from git-sim.
Hi again @paketb0te!
Wow cool! At a quick glance, I do like it. I like how it keeps just the global options in the __main__.py
and the subcommand options in the individual subcommand modules. Seems easier for the workflow to have all the stuff for each subcommand in one place.
I also like that we get rid of that big if
statement.
I need to take a deeper look into some of the functionality to fully understand how it works. I'll keep you posted on that but I like the direction this is going in.
Here are a few thoughts/questions:
-
So for the
Settings
, how should we decide what should be included there? We use default values both in the global options, subcommand options that are unique to each subcommand and also in some subcommand specific__init__()
methods. Would it just be anything in those categories that has a default value? -
One thing I want to make sure we can support is sub-sub commands. By that, I mean stuff like
$ git stash pop
and$ git stash push <filename>
. I hadn't looked into that with argparse yet but it was on my agenda. It would be great to know there is a clean way to do this with Typer before switching. -
This is a small thing, but in
animations.py
we'd need to be able to get the command name to include in the output image/video file here:image_file_name = "git-sim-log_" + t + ".jpg"
-
Also I think I like changing the
git_sim_subcommand.py
filenames to justsubcommand.py
, likelog.py
andstatus.py
(as well as the corresponding class names), just seems cleaner that way. But we can keepgit_sim_base_command.py
as is.
from git-sim.
So for the Settings, how should we decide what should be included there?
I'd have it store everything that is needed in the GitSimBaseCommand
in there (basically replacing args
- with the benefit of having type hints).
One thing I want to make sure we can support is sub-sub commands.
That is possible (and actually super easy), I have tried it already (see the subcommands section in the typer docs). You basically just create a new typer app for the subcommand and add it to the main app.
in animations.py we'd need to be able to get the command name
I think that should be fairly straightforward. Even though there does not seem to be an easy way for a function to know it's own name, we could always just add this to the Settings
class or pass it explicitly to handle_animations()
I like changing the git_sim_subcommand.py filenames
Main reason was that I wanted to look at the original files while creating the examples, but if you like it yeah we can of course rename them. Maybe we even move them to a commands
directory or something? 🤔
re: renaming, I think it would make sense to rename the GitSimCommand
classes to GitSimCommandScene
or CommandScene
(because they are the Scene objects for Manim :D )
What do you think?
from git-sim.
@initialcommit-io unfortunately this change would have to be done for all commands at once (at least I am not aware of a way to have arparse
and Typer
work at the same time to have a gradual change).
I am more than willing to do the work, but would like to make sure we are on the same page about this so that my time is not wasted :)
from git-sim.
@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.
from git-sim.
@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 :)
from git-sim.
@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?
from git-sim.
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 🤞
from git-sim.
@paketb0te Alright! I will do my best to take a look this weekend and merge the PR. Thanks again!
from git-sim.
@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?
from git-sim.
@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...
from git-sim.
@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.
from git-sim.
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)
from git-sim.
Re: the pydantic Settings, see #52 :)
from git-sim.
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.
from git-sim.
@initialcommit-io I only see the -e
shorthand for git-sim cherry-pick
, not for rebase
... a typo, or did I miss something?
from git-sim.
@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
from git-sim.
-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
from git-sim.
@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...
from git-sim.
@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.
from git-sim.
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
from git-sim.
@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,
- )
from git-sim.
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.
from git-sim.
@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.
from git-sim.
@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.
from git-sim.
@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...
from git-sim.
@paketb0te @abhijitnathwani I fixed the compatibility issues with older versions of Python (from 3.7 and up), refactored the manim config stuff as mentioned above, and made various other updates to release v0.2.3.
So I think our Typer setup is here pretty much completed for now. Closing this for now as the first version with Typer has been implemented and released.
Let's move various other/related conversations (like test suite) into "Discussions" or a relevant issue.
from git-sim.
Related Issues (20)
- Error in Dockerfile during 'apt update' HOT 3
- git-sim not recognized as command in M1 Apple HOT 3
- Clarify license HOT 5
- TypeError: can only concatenate str (not "ManimColor") to str HOT 4
- Fix arrangement of file names in columns
- Request: Make CHENGELOG.md HOT 1
- Font size is wrong HOT 5
- Cannot run git-sim to generate only images HOT 6
- Error on simple actions HOT 7
- Auto-completion is extremely slow HOT 6
- git submodule not support HOT 2
- Add -v, --version flag to display program version info HOT 2
- Implement PEP 621 (pyproject.toml) and migrate to "src" layout HOT 14
- Support gif animations HOT 1
- Extremely slow on larger repos HOT 9
- Arrow direction HOT 2
- AttributeError: module 'manim' has no attribute 'ArrowTriangleFilledTip' HOT 7
- Two branches without common commit? git-annex use case HOT 14
- --animate log in chronological order of the commits HOT 9
- Refine test suite HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from git-sim.