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

some improvement for systemctl3.py #173

Merged
merged 5 commits into from
Feb 2, 2024
Merged

Conversation

ladyrick
Copy link
Contributor

@ladyrick ladyrick commented Feb 2, 2024

  1. Fix SyntaxWarning: invalid escape sequence '\w'
企业微信截图_2097098b-93d6-4cbd-8170-08e43ae4041b
  1. Support multiple cmd path. Sometimes the "cat" command is not "/usr/bin/cat". So I change CAT_CMD to a list of candidates CAT_CMDS and search for the first existing path. Same change for LESS_CMD and TAIL_CMD.
image
  1. In log_unit_from, instead of spawnvp a new process for cat/tail/less, I use execvp to replace current process. By doing this, systemctl needn't to handle the exit code and errors.
    For example, before this PR, when I use systemctl log -f service to see the log and then use ctrl+c to exit, I will get an ugly error like this. But when this PR is accepted, the tail command will exit directly and no error will show.
企业微信截图_c8c2d440-3af7-4e26-b4a6-748bb7ce778e

@gdraheim gdraheim changed the base branch from master to develop February 2, 2024 14:04
@gdraheim
Copy link
Owner

gdraheim commented Feb 2, 2024

Please add get_exist_path to systemctl3.pyi

@ladyrick
Copy link
Contributor Author

ladyrick commented Feb 2, 2024

Please add get_exist_path to systemctl3.pyi

Done.

@gdraheim
Copy link
Owner

gdraheim commented Feb 2, 2024

Ahh, mypy asks for explicit return None in get_exist_path(paths):

@ladyrick
Copy link
Contributor Author

ladyrick commented Feb 2, 2024

Ahh, mypy asks for explicit return None in get_exist_path(paths):

Nice, All checks have passed this time.

@gdraheim gdraheim merged commit f203f3c into gdraheim:develop Feb 2, 2024
3 checks passed
@gdraheim
Copy link
Owner

gdraheim commented Feb 2, 2024

I will do some more checks locally before pushing to master though. In any case, thanks a lot for the patch, that is really helpful for the project to continue.

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