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

WIP: No pexpect #30

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

WIP: No pexpect #30

wants to merge 3 commits into from

Conversation

javierggt
Copy link
Contributor

@javierggt javierggt commented Jan 17, 2025

Description

This PR changes the getenv function so it does not use pexpect.

It is possible to replace all functionality so it doesn't use pexpect, but I'm not sure we want to. The reason is that in most cases the calling code can just replace the calls to ska_shell with a line calling subprocess, and that is less work than making ska_shell pexpect-free. In case where we care about a specific behavior, I don't see the point of forcing the current ska_shell interface and behavior. For example:

  • ska_shell has a logfile argument, but in this day and age we would prefer a logging.Logger or just a function.
  • ska_shell updates the output in real-time, which can be convenient. I did that in runasp using subprocess, but in most cases we do not need that.
  • ska_shell has an importenv argument to run_shell, but it can do that because it can make one last call to the shell instance to get the environment. In subprocess, it would require a bit of work.

If one does not care about keeping the exact arguments and behavior, then it is trivial to replace pexpect with subprocess.

Interface impacts

Testing

Unit tests

  • No unit tests
  • Mac
  • Linux
  • Windows

Independent check of unit tests by [REVIEWER NAME]

  • [PLATFORM]:

Functional tests

No functional testing.

@javierggt javierggt mentioned this pull request Jan 19, 2025
5 tasks
@javierggt javierggt changed the title No pexpect WIP: No pexpect Jan 20, 2025
@javierggt javierggt marked this pull request as draft January 20, 2025 14:49
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.

1 participant