Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for stash subcommands - push/pop/apply #60

Merged

Conversation

abhijitnathwani
Copy link
Contributor

Fixes #1

@paketb0te
Copy link
Contributor

@abhijitnathwani cool stuff!

I see you used an Enum to solve the subcommand problem (which is fine!), I think at some point we should refactor subcommands into their own Typer apps and then add them to the main Typer app as described in the Typer docs HERE (instead of adding them as commands to the main app).

The main advantage being that we can then nest as deep as we want, and it also removes the need for the extra Enum class.

So instead of

# stash.py

def stash():
    ...


# __main__.py

import typer
import git_sim.stash

app = typer.Typer()
app.command()(git_sim.stash.stash)

we'd have something like

# stash.py

import typer

app = typer.Typer()

@app.command()
def stash():
    ...


# __main__.py

import typer
import git_sim.stash

app = typer.Typer()
app.add_typer(git_sim.stash.app, name="stash")

Unfortunately I will not be available the next weeks, but in case you want to start with that, I trust you'll navigate the Typer docs just fine without me nagging all the time 😆

@abhijitnathwani
Copy link
Contributor Author

Hi @paketb0te
Thanks for the quick review! Makes sense to utilize Typer to its full potential. To be honest, I am using Typer for the first time and didn't know about all the features. I had referenced using Enum from some other command that was using it. I'll give it a go sometime next week and refactor all other commands using the Enum. We can create another issue to track it.

@initialcommit-io
Copy link
Contributor

@abhijitnathwani Thanks for this! I agree with the discussion above - I'll merge this for now and test it. And think we should do @paketb0te 's Typer suggestion for the subcommands going forward. @abhijitnathwani can you create a new issue for that?

FYI this is also my first time using Typer :D

@initialcommit-io initialcommit-io merged commit 915fbbe into initialcommit-com:main Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when running git-sim stash push/pop
3 participants