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

Add the ability to manually create ParsedPaths (+ cleanup) #11029

Merged
merged 10 commits into from
Feb 1, 2024

Conversation

doonv
Copy link
Contributor

@doonv doonv commented Dec 19, 2023

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.
  • Improved ReflectPath error handling.

Migration Guide

  • bevy::reflect::AccessError has been refactored.

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

@nicopap nicopap self-requested a review December 19, 2023 20:12
@alice-i-cecile
Copy link
Member

@viridia opinions on whether we should do this?

@viridia
Copy link
Contributor

viridia commented Dec 19, 2023

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

@ItsDoot ItsDoot added C-Usability A simple quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types labels Dec 20, 2023
@james7132 james7132 self-requested a review December 23, 2023 17:43
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to rename the error type, but this seems like a reasonable API to expose and the engineering is solid.

@doonv doonv requested a review from MrGVSV January 26, 2024 11:16
@nicopap
Copy link
Contributor

nicopap commented Jan 29, 2024

I reviewed the code, I posted my feedbacks as a PR on the doonv/bevy repo, see doonv#2

* Split the error code into its own module
* Use a newtype for the offset, instead of `Option<usize>` (16 bytes),
  we have a `usize` (8 bytes).
* Make a few error types private, so that they don't need to be
  documented.
* Make `AccessError` a `struct` with an `access` and `offset` fields.
@doonv
Copy link
Contributor Author

doonv commented Jan 29, 2024

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

@nicopap
Copy link
Contributor

nicopap commented Jan 30, 2024

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

@alice-i-cecile
Copy link
Member

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.

@doonv
Copy link
Contributor Author

doonv commented Jan 30, 2024

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

@doonv
Copy link
Contributor Author

doonv commented Jan 31, 2024

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?

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 1, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 1, 2024
Merged via the queue into bevyengine:main with commit b1a2d34 Feb 1, 2024
23 checks passed
tjamaan pushed a commit to tjamaan/bevy that referenced this pull request Feb 6, 2024
…e#11029)

# 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 `ParsedPath`s from their
elements, without the need of string parsing.
- Improved `ReflectPath` error handling.

## Migration Guide

-  `bevy::reflect::AccessError` has been refactored.

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

---------

Co-authored-by: Nicola Papale <nicopap@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types C-Usability A simple quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants