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

fix: TermExec cmd with config.shell = function #467

Merged
merged 1 commit into from
Aug 2, 2023
Merged

Conversation

asror1
Copy link
Contributor

@asror1 asror1 commented Jul 31, 2023

fixes(#466)

This minor change personally resolves the issue I was facing, although potentially there might be other instances in the code where the shell option is assumed to be a string, I have not looked through the codebase for other occurrences though.

Copy link
Owner

@akinsho akinsho left a comment

Choose a reason for hiding this comment

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

@asror1 thanks for the PR this looks good, but as you say might miss places where else shell is expected to be a string. I think it would be better to re-assign the value inside of the new or the spawn function similar to how the height and width are derived and stored from functions so that the function is only evaluated once on creation

@asror1
Copy link
Contributor Author

asror1 commented Aug 1, 2023

it would be better to re-assign the value inside of the new or the spawn function similar to how the height and width are derived and stored from functions so that the function is only evaluated once on creation

I've looked through the codebase and the spawn function, or rather the inner __spawn function is already evaluating the shell function before using it.

function Terminal:__spawn()
  local cmd = self.cmd or config.get("shell")
  if type(cmd) == "function" then cmd = cmd() end
  ...

I believe the error was truly just coming the local get_newline_chr() function in the terminal module that is using the shell option directly from config, rather than having it passed in as a parameter. But I'm not sure if it would make sense to have get_newline_chr() accept the shell as parameter, since it is only ever used in the with_cr local func, which in turn is only ever used in Terminal:send method.

Copy link
Owner

@akinsho akinsho left a comment

Choose a reason for hiding this comment

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

Will merge as is for now since this fixes the issue for now

@akinsho akinsho merged commit 83871e3 into akinsho:main Aug 2, 2023
This was referenced Aug 2, 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.

2 participants