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

Add gitArgs as Cmd Argument #11

Merged
merged 3 commits into from
Jun 15, 2020
Merged

Add gitArgs as Cmd Argument #11

merged 3 commits into from
Jun 15, 2020

Conversation

scheeles
Copy link
Contributor

This enables the possibility to set e.g. --no-pager flag

@capnfabs
Copy link
Owner

Wowwwwwww I didn't know that this was an argument to git and not to git diff!

Thanks for the PR, @scheeles! I'm going to finish a refactor of some internals in the coming week, and then will merge this after and release it into the next version.

I've got one outstanding UX question I'd like your thoughts on -- it seems to me that naming this gitargs is ambiguous in that it's unclear whether it's used for checking out certain commits (e.g. git [gitargs] checkout) or for the diff command (the change you've made for this PR here).

What do you think instead about a UX where you supply the gitargs and the diffargs and the diff statement in one argument instead? e.g.

  • instead of grouse --gitargs='--no-pager' --diffArgs='--stat'
  • this: grouse --diffCmd='--no-pager diff --stat'?

I see pros and cons to both approaches.

@capnfabs
Copy link
Owner

Otherwise, it looks like there's only a handful of these program-level args that would be relevant for grouse, I guess we could just add --no-pager as an argument to grouse directly. 🤔

@capnfabs capnfabs added this to the 0.2 milestone Jun 14, 2020
@scheeles
Copy link
Contributor Author

Not sure if --diffCmd='--no-pager is misleading, but I am fine with adding only --no-pager.

# Conflicts:
#	internal/pkg/argparsing.go
#	internal/pkg/argparsing_test.go
@capnfabs capnfabs merged commit 83bda36 into capnfabs:dev Jun 15, 2020
@capnfabs
Copy link
Owner

Alright, merged this + will refactor to take the --no-pager argument directly. Thanks for your contribution!

capnfabs added a commit that referenced this pull request Jun 15, 2020
As per discussion on #11
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.

2 participants