-
-
Notifications
You must be signed in to change notification settings - Fork 207
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
Implement FromStr for NodePath #201
Conversation
Nothing generally against it, but what is your motivating use case, which isn't already satisfied by |
It's more clear/explicit since |
It needs bors try |
tryBuild succeeded: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, forgot to send the actual comment.
godot-core/src/builtin/node_path.rs
Outdated
type Err = (); | ||
|
||
fn from_str(path: &str) -> Result<Self, ()> { | ||
Ok(Self::from(&GodotString::from(path))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you call Self::from(path)
directly?
There is a From<&str>
that we can reuse 🙂
86f09f5
to
88c7c64
Compare
godot-core/src/builtin/node_path.rs
Outdated
@@ -60,6 +61,15 @@ impl From<&str> for NodePath { | |||
} | |||
} | |||
|
|||
// TODO: Check if it's required to implement Err | |||
impl FromStr for NodePath { | |||
type Err = (); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, something else -- this should probably be Infallible
, no? ()
means there is a value for errors, however we want to have no value as it always succeeds.
This is also what String::from_str
does.
You can then remove the TODO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, that makes sense to use Infallible
there too.
88c7c64
to
0556d52
Compare
Thanks a lot! bors r+ |
Build succeeded: |
Implements
FromStr
forNodePath
.