Skip to content

Expose bin_path after parsing #139

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Expose bin_path after parsing #139

wants to merge 7 commits into from

Conversation

yanganto
Copy link

fix #137

@yanganto
Copy link
Author

Hi @tertsdiepraam
The CI fail of Clippy is nothing about the change in the PR. If you want me to fix it in this PR, I am happy to do so.

@yanganto
Copy link
Author

yanganto commented Jun 9, 2025

Hi @tertsdiepraam
How do you think about this PR?

The bin name is exposed as PathBuf now.

Additionally, when a user developing with uutils-args with cargo run -- -h the bin name in the doc will show the bin name, and not the path to the bin.

If you have any further concern, I am happy to work on it. 😄

tertsdiepraam
tertsdiepraam reviewed Jun 9, 2025
View reviewed changes
Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

Hi! Looks good!

There are some things we should probably think about after this change. Maybe it makes sense to not return a tuple but a struct like this:

struct Parsed<S> {
    bin_name: PathBuf,
    operands: Vec<OsString>,
    settings: S,
}

Happy to accept this PR as it is too, but if you agree we can still make that change here.

@yanganto yanganto requested a review from tertsdiepraam June 11, 2025 15:54
@yanganto
Copy link
Author

Hi, @tertsdiepraam

The struct Parsed<S> is implemented, and also fn trim(self) -> S for Parsed<S> is provided, such that in the happy case, user can straightforwardly get the settings.

If there is anything better to have, I am happy to add it into the PR. 😃

@yanganto
Copy link
Author

Hi, @tertsdiepraam

Does anything need to change in the PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tertsdiepraam tertsdiepraam Awaiting requested review from tertsdiepraam

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Expose bin_name after parsing
2 participants
@yanganto @tertsdiepraam