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 option to specify other shell(Arg) #3299

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

J-Kappes
Copy link

@J-Kappes J-Kappes commented Feb 2, 2024

  • PR Description

  • if os.shell is not empty it replaces the default "bash" shell and os.shellArg replaces the default "-c" argument.

  • started writing test but don't know how to check stdout or command log.

  • Please check if the PR fulfills these requirements

  • Cheatsheets are up-to-date (run go generate ./...)
  • Code has been formatted (see here)
  • Tests have been added/updated (see here for the integration test guide)
  • Text is internationalised (see here)
  • Docs (specifically docs/Config.md) have been updated if necessary
  • You've read through your own file changes for silly mistakes etc

Copy link
Contributor

@karimkhaleel karimkhaleel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. I tested it out, and it seems to work as advertised. Left a couple of comments that I think could help.

pkg/gui/gui.go Outdated
Comment on lines 538 to 542
shell := gui.UserConfig.OS.Shell
if shell != "" {
osCommand.Platform.Shell = shell
osCommand.Platform.ShellArg = gui.UserConfig.OS.ShellArg
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to move this logic into NewOsCommand? We want all os commands to inherit that setting, right? Or do we only want the command runner for custom commands to use the shell and shellArgs from the userconfig?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I don't know how to access config.OS from there, though 😕

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, I think it would make more sense to pass the OSConfig struct into oscommands.GetPlatform and get that function to extract the shell and shellArg values from there and use them there directly.

Sounds good. I don't know how to access config.OS from there, though 😕

You would be able to access the config through the Common struct that's passed into NewOsCommand function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure, but perhaps an integration test is not the right thing for this feature? Maybe this should just be a unit test in pkg/commands/oscommands/os_test.go

},
"shell": {
"type": "string",
"description": "Shell and ShellArg are the shell and corresponding argument to be used for executing custom commands.\nSee https://github.com/jesseduffield/lazygit/blob/master/docs/Config.md#custom-command-for-copying-to-clipboard"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The URL here needs to point to the "Custom Shell for executing arbitrary commands" section of the Config.md file.

Copy link
Contributor

@karimkhaleel karimkhaleel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Just one last comment.

Comment on lines +20 to +23
if osConfig.Shell != "" {
platform.Shell = osConfig.Shell
platform.ShellArg = osConfig.ShellArg
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think at this point, we should probably just consider setting bash and -c as default parameters for Shell and ShellArg in the user_config file. That way, we can just directly take the values from osConfig and use them in the Platform literal.

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.

None yet

2 participants