[go: up one dir, main page]
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.

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

Implement FromStr and TryFrom for ParsedPath #14438

Closed
Multirious opened this issue Jul 22, 2024 · 2 comments · Fixed by #15180
Closed

Implement FromStr and TryFrom for ParsedPath #14438

Multirious opened this issue Jul 22, 2024 · 2 comments · Fixed by #15180
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! X-Uncontroversial This work is generally agreed upon

Comments

@Multirious
Copy link
Contributor

What problem does this solve or what need does it fill?

ParsedPath is parsed from strings and so can implement FromStr and any of the TryFrom string variants.
This let users to use .parse() or library author to create functions using TryInto<ParsedPath> for ergonomic reasons.

What solution would you like?

Implment any of:

  • FromStr
  • TryFrom<&str>
  • TryFrom<String>
  • more string variants...

What alternative(s) have you considered?

Use ParsedPath::parse() or ParsedPath::parse_static() directly.
@Multirious Multirious added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Jul 22, 2024
@alice-i-cecile alice-i-cecile added A-Assets Load files from disk to use for things like images, models, and sounds C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! X-Uncontroversial This work is generally agreed upon D-Straightforward Simple bug fixes and API improvements, docs, test and examples and removed C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Jul 22, 2024
@alice-i-cecile
Copy link
Member

I think FromStr is my preference, but I'd be curious to hear more about which one is idiomatic and why.

Definitely think this should exist!

@soqb
Copy link
Contributor
soqb commented Jul 22, 2024

iirc FromStr only really exists as a remnant from (no pun intended) before TryFrom<&str> existed.

i think the typical solution is to implement both but there's an argument for not implementing FromStr since it's the less "flexible" trait.

i think TryFrom<&str> is the only parameter type which makes much sense considering the current ParsedPath api.

crvarner added a commit to crvarner/bevy that referenced this issue Sep 13, 2024
@crvarner
- implements ParsedPath::try_from<&str>
b939675 
- resolves bevyengine#14438
@crvarner crvarner mentioned this issue Sep 13, 2024
Merged
github-merge-queue bot pushed a commit that referenced this issue Sep 13, 2024
@crvarner
ParsedPath::try_from<&str> (#15180)
cf55e6c 
# Objective

- implements ParsedPath::try_from<&str>
- resolves #14438

## Testing

- Added unit test for ParsedPath::try_from<&str>

Note: I don't claim to be an expert on lifetimes! That said I think it
makes sense that the error shares a lifetime with input string as deeper
down it is used to construct it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! X-Uncontroversial This work is generally agreed upon
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

ParsedPath::try_from<&str> crvarner/bevy
3 participants
@alice-i-cecile @soqb @Multirious