Fish support (closes #8, closes #27)

[jdelStrother]

This adds fish support via scmpuff init -s --shell=fish. The fish scripts are based on work done by https://github.com/arbelt/fish-plugin-scmpuff, though I've tweaked them to better match the regular scmpuff behaviour.

One downside is that it means you'll get failing feature-tests unless you have fish installed. I couldn't spot a clean way of skipping the fish examples, but let me know if you think that's important.

[mroth]

Ooh, exciting! At first glance this looks very clean, well done. I'll try to do a detailed review in the near future (next few weeks at work are unfortunately crunch time), but in general I think combining this with figuring out #57 will make for a good v0.5.0 milestone target.

One downside is that it means you'll get failing feature-tests unless you have fish installed. I couldn't spot a clean way of skipping the fish examples, but let me know if you think that's important.

I'm not too worried about that, now that I have a vscode devcontainer setup (de534d9 ) it should be easy enough to add fish binary to it. I may eventually extract that to a generic Docker container for running integration tests to make life easier for folks who don't use vscode as well.

[jdelStrother]

I'm not too worried about that, now that I have a vscode devcontainer setup (de534d9 ) it should be easy enough to add fish binary to it. I may eventually extract that to a generic Docker container for running integration tests to make life easier for folks who don't use vscode as well.

I'm not that familiar with vscode's devcontainers, but have added fish to the dockerfile - seems to work ok...

[mroth]

Nice. I'm really impressed with the clean CI integration too. Some initial review comments on minor implementation details.

The other primary thing I'd like to discuss is scmpuff init -s --shell=fish feels a bit unwieldy from a UX perspective. Thinking out loud here, but what do you think of deprecating the old -s, --show entirely, and replacing it with this new -s, --shell. The default flag value could remain sh, so it would remain reverse compatible for anyone with a scmpuff init -s flag in their bash profile or whatnot.

The one old use case this would break (I believe) would be anyone using the long form --show, but I think we could still preserve that without breakage even by making a deprecated --show Flag pointing to the same function, and marking it Hidden so it doesn't show up in help.

What do you think?

[jdelStrother]

The other primary thing I'd like to discuss is scmpuff init -s --shell=fish feels a bit unwieldy from a UX perspective. Thinking out loud here, but what do you think of deprecating the old -s, --show entirely, and replacing it with this new -s, --shell. The default flag value could remain sh, so it would remain reverse compatible for anyone with a scmpuff init -s flag in their bash profile or whatnot.

The one old use case this would break (I believe) would be anyone using the long form --show, but I think we could still preserve that without breakage even by making a deprecated --show Flag pointing to the same function, and marking it Hidden so it doesn't show up in help.

What do you think?

Sounds good, I'll update it. I'd wondered about making it default to guessing the value from $SHELL, but maybe that's too much magic.

[jdelStrother]

... I've actually found the --show -> --shell change a little tricky, see what you think.

Deprecating show seems easy:

	// --show (deprecated in favor of --shell)
	InitCmd.Flags().BoolVar(
		&outputScript,
		"show", false,
		"Output scmpuff initialization scripts",
	)
	InitCmd.Flags().MarkHidden("show")

But I've been trying to implement the shell flag with something like this:

	// --shell
	InitCmd.Flags().StringVarP(
		&shellType,
		"shell", "s", "",
		"Set shell type - 'sh' (for bash/zsh), or 'fish'",
	)
	InitCmd.Flag("shell").NoOptDefVal = "sh"

NoOptDefVal is necessary to allow the arg-less scmpuff init -s or scmpuff init --shell to continue working. However, it breaks scmpuff init --shell foo - instead, scmpuff init --shell=foo is required. (due to a long-standing cobra bug)

I've added Arg validation so at least scmpuff init --shell fish doesn't silently ignore the problem, but it doesn't seem ideal.

[mroth]

NoOptDefVal is necessary to allow the arg-less scmpuff init -s or scmpuff init --shell to continue working. However, it breaks scmpuff init --shell foo - instead, scmpuff init --shell=foo is required. (due to a long-standing cobra bug)

I've added Arg validation so at least scmpuff init --shell fish doesn't silently ignore the problem, but it doesn't seem ideal.

Weird bug. That said, is there a reason we need to use pflag NoOptDefVal here and not just make sh the default for the "shell" StringVarP flag (instead of the empty string) entirely? I think my brain is failing at understanding something obvious here. Oh, I get it now after playing with it, this is to preserve outputting help when init has no args. Will update once I think more.

(Additionally, your idea of parsing $SHELL to set the default flag value could also work quite well I think, with a fallback to sh if detection fails, but perhaps let's resolve the overall UX first. )

[mroth]

Okay, I tried cleaning up the help string a bit in 963104b to try to make the setup instructions show up in more places. Play around with it and let me know how it feels to you. With this, I think it may be acceptable to live with the error condition if someone omits the =. If it causes user confusion, we could always to add a more complex parsing versus cobra.NoArgs to explain the edge case better. What do you think?

[jdelStrother]

Yep, looks good to me

[mroth]

Okay, I refactored the script collection handling a little bit to make it more robust for the future, automatically lowercase the passed shell name (to make it match easier in case someone is outputting from something strange), and added a rudimentary default shell detection based on your idea (we should probably still prompt folks to manually specify in the setup instructions, but I think having this detection makes the simple -s case more ideal?).

I'm also going to merge #64 and update the version of Cobra, and rebase so we can do manual verification on the most recent version.

Take a look and play around with it and see if I broke anything. I'm also curious if we are missing anything in the UX/documentation that could make things clearer for folks.

[jdelStrother]

Looks good. I did run into some problems with rake install on my M1 Mac Mini which I've fixed here - 552076f - but can submit that as a separate PR if you'd prefer

[euoia]

Having used SCM Breeze for years and recently switching to Fish shell, I'm keeping a close eye on this. I'm using shinriyo/breeze but found enough differences from SCM Breeze that I would like to try an alternative. I like the philosophy of scmpuff and will happily try this out and provide feedback once merged. Thanks!

[mroth]

Looks good. I did run into some problems with rake install on my M1 Mac Mini which I've fixed here - 552076f - but can submit that as a separate PR if you'd prefer

Wow, that's bizarre. I don't have a M1 Mac yet so this was good to learn! (I think it's fine to bundle in this PR).

[mroth]

Having used SCM Breeze for years and recently switching to Fish shell, I'm keeping a close eye on this. I'm using shinriyo/breeze but found enough differences from SCM Breeze that I would like to try an alternative. I like the philosophy of scmpuff and will happily try this out and provide feedback once merged. Thanks!

Thanks @euoia. I think this is ready to be merged soon, I hopefully will try to find some time this coming weekend. Past that point, I think it would be good to wrap up #57 and then cut a new major version release.

(@euoia if you let me know your OS and architecture, I can also build a pre-release snapshot binary for you if you'd like to test in advance.)

[euoia]

(@euoia if you let me know your OS and architecture, I can also build a pre-release snapshot binary for you if you'd like to test in advance.)

MacOS Monterey, Intel. Thanks!

[mroth]

MacOS Monterey, Intel. Thanks!

@euoia snapshot binary attached: scmpuff_v0.4.0-next_macOS_x64.tar.gz

[euoia]

@mroth thanks! Testing as of this morning, everything seems great so far. I noticed you didn't alias gc for git commit, presumably because it conflicts with an existing tool. That's fine, I added the alias myself, but may be worth noting in the page on differences to SCM Breeze. The list of aliases for SCM Breeze seems to be here:
https://github.com/scmbreeze/scm_breeze/blob/master/lib/git/aliases.sh
https://github.com/scmbreeze/scm_breeze/blob/0a687dcfe56967b96522efcc7db9f6c3a24214c1/git.scmbrc.example