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

Document query errors #8692

Merged
merged 8 commits into from
May 30, 2023
Merged

Document query errors #8692

merged 8 commits into from
May 30, 2023

Conversation

Selene-Amanita
Copy link
Member

@Selene-Amanita Selene-Amanita commented May 27, 2023

Objective

Add documentation to Query and QueryState errors in bevy_ecs (QuerySingleError, QueryEntityError, QueryComponentError)

Solution

  • Change display message for QueryEntityError::QueryDoesNotMatch: this error can also happen when the entity has a component which is filtered out (with Without<C>)
  • Fix wrong reference in the documentation of Query::get_component and Query::get_component_mut from QueryEntityError to QueryComponentError
  • Complete the documentation of the three error enum variants.
  • Add examples for QueryComponentError::MissingReadAccess and QueryComponentError::MissingWriteAccess
  • Add reference to QueryState in QueryEntityError's documentation.

Migration Guide

Expect QueryEntityError::QueryDoesNotMatch's display message to change? Not sure that counts.

@github-actions
Copy link
Contributor

Welcome, new contributor!

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

@djeedai djeedai added C-Docs An addition or correction to our documentation A-ECS Entities, components, systems, and events labels May 27, 2023
@Selene-Amanita
Copy link
Member Author

(Just in case, here is the file I used to test the example:)

use bevy::app::App;
use bevy::ecs::{prelude::*, system::QueryComponentError};

#[derive(Component)]
struct OtherComponent;

#[derive(Component)]
struct RequestedComponent;

#[derive(Resource)]
struct SpecificEntity {
    entity: Entity,
}

fn get_missing_read_access_error(query: Query<&RequestedComponent>, res: Res<SpecificEntity>) {
    if let Err(QueryComponentError::MissingReadAccess) = query.get_component::<OtherComponent>(res.entity) {
        println!("query doesn't have read access to OtherComponent");
    }
}

fn get_missing_write_access_error(mut query: Query<&RequestedComponent>, res: Res<SpecificEntity>) {
    if let Err(QueryComponentError::MissingWriteAccess) = query.get_component_mut::<RequestedComponent>(res.entity) {
        println!("query doesn't have write access to RequestedComponent");
    }
}

fn main() {
    App::new()
        .add_startup_system(setup)
        .add_system(get_missing_read_access_error)
        .add_system(get_missing_write_access_error.after(get_missing_read_access_error))
        .run();
}

fn setup(
    mut commands: Commands,
) {
    let entity = commands.spawn((RequestedComponent, OtherComponent)).id();
    commands.insert_resource(SpecificEntity{entity})
}```

crates/bevy_ecs/src/query/state.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/query/state.rs Show resolved Hide resolved
crates/bevy_ecs/src/query/state.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/query.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/query.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/query.rs Outdated Show resolved Hide resolved
Selene-Amanita and others added 4 commits May 27, 2023 16:38
Co-authored-by: harudagondi <giogdeasis@gmail.com>
Co-authored-by: harudagondi <giogdeasis@gmail.com>
Co-authored-by: harudagondi <giogdeasis@gmail.com>
@Selene-Amanita
Copy link
Member Author

Applied rustfmt (sorry I thought it was applied automatically somehow)

Copy link
Member

@harudagondi harudagondi left a comment

Choose a reason for hiding this comment

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

looks good to me

@harudagondi harudagondi 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 May 28, 2023
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.

Great work, these are much better. Thanks for improving these :)

@alice-i-cecile alice-i-cecile enabled auto-merge May 29, 2023 15:44
@alice-i-cecile
Copy link
Member

@Selene-Amanita it looks like your doc tests are failing: can you fix those up?

auto-merge was automatically disabled May 29, 2023 17:23

Head branch was pushed to by a user without write access

@Selene-Amanita
Copy link
Member Author

Selene-Amanita commented May 29, 2023

@alice-i-cecile I fixed the check-docs check, I think.

I couldn't run cargo miri ("the 'miri' component which provides the command 'cargo-miri' is not available for the 'stable-x86_64-unknown-linux-gnu' toolchain") so hopefully this will work out too.

Btw should I use an assert inside the system in the doc test, to check that this scenario does yield the expected error?

@alice-i-cecile
Copy link
Member

alice-i-cecile commented May 29, 2023

Yeah, I would put an assert inside the doc test: it'll act as both clarification and as an extra layer of safety for our ECS code.

Miri won't be doing anything special here: just making sure the tests all pass, so you don't need to worry about it.

@Selene-Amanita
Copy link
Member Author

Selene-Amanita commented May 29, 2023

@alice-i-cecile

Okay! The checks did pass this time but I added assert_eq in 5559ce7.
Questions/remarks:

  • cargo fmt doesn't apply formating to tests/examples in doc, right? Should I do something about it?
  • I kept bevy_ecs::system::assert_is_system(get_missing_write_access_error);, should I delete it?
  • I kept the println! and expanded it a bit for clarity
  • I had to derive PartialEq and Debug for RequestedComponent otherwise I would have to make convoluted code

@alice-i-cecile
Copy link
Member

cargo fmt doesn't apply formating to tests/examples in doc, right? Should I do something about it?

Mostly we just suffer here :( The tooling for doc tests is really subpar in general, which is a shame because they're very useful. Manually matching it is way more work than it's worth though.

I kept bevy_ecs::system::assert_is_system(get_missing_write_access_error);, should I delete it?

That's fine.

I kept the println! and expanded it a bit for clarity

Great!

I had to derive PartialEq and Debug for RequestedComponent otherwise I would have to make convoluted code

Good choice.

Merging!

@alice-i-cecile alice-i-cecile enabled auto-merge May 30, 2023 14:27
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 30, 2023
Merged via the queue into bevyengine:main with commit ca81d3e May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Docs An addition or correction to our documentation M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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.

4 participants