Add core Error to InvalidDirectionError

[shafferchance]

Objective

Fixes #17761

Solution

• Added core error to InvalidDirectionError

Testing

• Did you test these changes? If so, how?
  • An added test that pulls in anyhow as a dev dependency to ensure the conversion is accounted for in creation via From
• Are there any parts that need more testing?
  • I'm unsure but probably not due to being a trivial change
• How can other people (reviewers) test your changes? Is there anything specific they need to know?
  • I did not try a fully built version of Bevy. Relied purely on tests.
• If relevant, what platforms did you test these changes on, and are there any important ones you can't test?
  • Only windows

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

[shafferchance]

I used core::error::Error to follow the example of other std usage in the file. Please let me know if this was not the right thing to do.

[shafferchance]

Looks like I'm having some trouble running locally for format. Since this impacts only a single file I opted to format that single file via explicitly passing it to cargo fmt. However, rust-lang/rustfmt#5975 is something to be aware of for people working from windows machines.

[SpecificProtagonist]

I used core::error::Error to follow the example of other std usage in the file. Please let me know if this was not the right thing to do.

Using core where possible is indeed correct! That said, Bevy generally uses thiserror for convenient error impls (like this). Doing that here would let us get rid of the manual (and unspecific) Display implementation :)

[SpecificProtagonist]

This addition to the test isn't really useful

[shafferchance]

I can understand that, so if that's the case, should there not be a test then since this was a missing trait bound? Or should we shift it to interacting with the error interface and check the message? Or is there a better way I'm not setting?

[shafferchance]

Ah, okay, that makes much more sense, I wanted to fill the display piece, I didn't know what to put exactly. I'll switch it over, thanks!

[shafferchance]

@SpecificProtagonist sorry it took a bit to do the simple push. I removed the test per your feedback. I'm curious, do we trust that thiserror will handle that case?

[SpecificProtagonist]

No problem :)
This still uses a custom display implementation that prints the same message for all variants, you can remove it if you add #[error("Description of the error here pls")] to the variants.

I'm curious, do we trust that thiserror will handle that case?

Could you elaborate? The addition to the test was doing two things:

• Test that downcasting works in anyhow. That's unrelated to this specific error or even to Bevy.
• Double-checks that InvalidDirectionError implements Error. This is a static property, so if we would want to check it, we would do so statically. But let's not add an additional check here: The Error impl comes from an explicit derive, so that check would just re-state that. Tests are there to prevent accidental regressions, but it would be hard to accidentally remove the derive. We don't also check that this type implements Debug or Display or PartialEq (you could add an Eq derive while you're at it), nor do we do these checks for all other types for the same reason.

[shafferchance]

Thank you for the explanation. After pondering a bit longer, I see what you mean now. I think the question was more about whether we were outsourcing trust. However, with your explanation, I understand it isn't "outsourcing" but rather "anyhow," which is not related to Bevy and should not be tied to testing directly, especially for something static.

As for the changes, a trait conflict was resolved by removing the other display defined for the Error. Granted, I think this will be more descriptive now instead of the message that combined all three. Let me know if we should re-add that display so we can look at options to get around the conflict.