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

Pass non-str as Path arguments #4

Closed
wants to merge 1 commit into from

Conversation

fosskers
Copy link
Contributor

@fosskers fosskers commented Feb 3, 2022

This PR allows anything that has AsRef<Path> to be passed as args to various Config methods. Luckily str has that impl, so existing code should need no changes, and this PR should only cause a minor version bump.

@fosskers fosskers mentioned this pull request Feb 3, 2022
10 tasks
@Morganamilo
Copy link
Owner

Hm so I think originally went with strings because the config file is passed as an argument and not an actual path. And I believe this was one of the first rust things I did.

While you say existing code shouldn't need to change. As you can see by your own change to the tests, some code will have to change. This mean's It will have to be v2 which I'm not too happy about but it's probably the way to go.

@Morganamilo
Copy link
Owner

Also as Command takes OsStr maybe we should too instead of path

@Morganamilo
Copy link
Owner

I think This should be OsStr instead. However I would still like it if there was a way to not have to use turbo fish for all None's. Though that may be a non issue as if you're passing just none's you're probably using ::new() instead.

@Morganamilo
Copy link
Owner

Also also for the turbo fish I think it should be :: iinstead of str. This should mean it doesn't have to do any conversion and is on paper better.

@Morganamilo
Copy link
Owner

I implemented this myself as well as some clean up. let me know if that's all okay.

@fosskers
Copy link
Contributor Author

Forgive the slow response, my FOSS days are staggered to every-other-day.

The reason I submitted this change in the first place is because str -> Path is a total conversion but Path -> str has to go through UTF8 checks (thus landing you in Result-land). If you already had a Path on hand, then you could pass it right into your functions if they're decorated with P: AsRef<Path>.

Now thanks to Rust being awesome, Path has AsRef<OsStr>, so that should work just as well. Let me test it.

@fosskers
Copy link
Contributor Author

Seems to work! If you're able to make a release, then I'll put out r2d2-alpm.

@fosskers fosskers closed this Feb 10, 2022
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