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

Remove trim on result of exec command #475

Merged
merged 2 commits into from
Apr 18, 2024

Conversation

kimbob13
Copy link
Contributor

Hello,
I'm using this library to execute command on remote host,
and I want the result to be exactly same as what executed locally.

I think trimming process is a choice of user of library side, not what the library should perform.
Therefore, this PR removes trim method on stdout and stderr results.

Copy link
Owner

@steelbrain steelbrain left a comment

Choose a reason for hiding this comment

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

Hi @kimbob13! Thanks for the PR. Would you be interesting in adding a new noTrim: true | false option instead?

This allows us to cater to most people's needs, as they don't need the extra newline at the end, but for the people who need it, they can turn on the option instead.

@kimbob13
Copy link
Contributor Author

Thanks for the suggestions! I'll add option instead.

@kimbob13
Copy link
Contributor Author

@steelbrain I've changed to option approach and force pushed.

Copy link
Owner

@steelbrain steelbrain left a comment

Choose a reason for hiding this comment

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

Minor change and then we're good to go!

src/index.ts Outdated Show resolved Hide resolved
Co-authored-by: Anees Iqbal <hello@aneesiqbal.ai>
@kimbob13
Copy link
Contributor Author

Warmly reminder, commit added!

@steelbrain
Copy link
Owner

Thank you for working on this! I’ll add some tests and make some doc changes before releasing. (Also happy to accept PRs for them )

@steelbrain steelbrain merged commit c4bc27f into steelbrain:main Apr 18, 2024
@steelbrain
Copy link
Owner

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