bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Add the ability to manually create ParsedPaths (+ cleanup)

Open doonv opened this issue 2 years ago • 2 comments

Objective

I'm working on a developer console plugin, and I wanted to get a field/index of a struct/list/tuple. My command parser already parses member expressions and all that, so I wanted to construct a ParsedPath manually, but it's all private.

Solution

Make the internals of ParsedPath public and add documentation for everything, and I changed the boxed slice inside ParsedPath to a vector for more flexibility.

I also did a bunch of code cleanup. Improving documentation, error messages, code, type names, etc.


Changelog

  • Added the ability to manually create ParsedPaths from their elements, without the need of string parsing.

Migration Guide

  • Replaced bevy::reflect::AccessError, with bevy::reflect::access::Error.

That should be it I think, everything else that was changed was private before this PR.

doonv avatar Dec 19 '23 19:12 doonv

@viridia opinions on whether we should do this?

alice-i-cecile avatar Dec 19 '23 22:12 alice-i-cecile

@alice-i-cecile Different kind of "path" I think. I don't think we're talking about filesystem paths here.

viridia avatar Dec 19 '23 22:12 viridia

I reviewed the code, I posted my feedbacks as a PR on the doonv/bevy repo, see https://github.com/doonv/bevy/pull/2

nicopap avatar Jan 29 '24 18:01 nicopap

@nicopap Why did you decide to make the inner error types private?

doonv avatar Jan 29 '24 18:01 doonv

I prefer making them private because:

  • It's less things to document.
  • It's less confusing to users (which may be victim of information overload when exposed with many types)
  • It allows changing internals without breaking user code. Have you noticed how annoying it is to update the tests after changing the error types? Well, it would be equally annoying for users of the error type.

But of course there are advantages to exposing them:

  • It allows handling path errors in a very customizable way. For your use case, actually, it's quite a useful feature to have.

I think it's probably better to expose them, now that I write this down. But eh, I don't want to write the doc :P

nicopap avatar Jan 30 '24 11:01 nicopap

No strong feelings on whether or not those are public :) @nicopap if you're happy enough with this now, leave an approval and I'll merge this in.

alice-i-cecile avatar Jan 30 '24 14:01 alice-i-cecile

I certainly want them public for my use case, I'll make a commit which makes them public.

doonv avatar Jan 30 '24 17:01 doonv

Refactor changes:

  • AccessErrorKind, and TypeKind are now public.
  • AccessErrorKind no longer implements Display, and Error
  • AccessError now manually implements Error and Display
  • Removed Offset, now using Option<usize> again (The optimization used by Offset is quite unnecessary imo, it's the difference between 64 and 72 bits on a 64-bit OS)
  • A few misc changes

@nicopap can you review this now?

doonv avatar Jan 31 '24 18:01 doonv