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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Mention creation of disjoint Querys with Without<T> in conflicting access Panic #2413

Closed
wants to merge 2 commits into from
Closed

[Merged by Bors] - Mention creation of disjoint Querys with Without<T> in conflicting access Panic #2413

wants to merge 2 commits into from

Conversation

MinerSebas
Copy link
Contributor

@MinerSebas MinerSebas commented Jun 29, 2021 •
edited
Loading

Objective

Beginners semi-regularly appear on the Discord asking for help with using QuerySet when they have a system with conflicting data access.
This happens because the Resulting Panic message only mentions QuerySet as a solution, even if in most cases Without<T> was enough to solve the problem.

Solution

Mention the usage of Without<T> to create disjoint queries as an alternative to QuerySet

Open Questions

  • Is disjoint a too technical/mathematical word?
  • Should Without<T> be mentioned before or after QuerySet?
    • Before: Using Without<T> should be preferred and mentioning it first reinforces this for a reader.
    • After: The Panics can be very long and a Reader could skip to end and only see the QuerySet

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jun 29, 2021
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Jun 29, 2021
@NathanSWard
Copy link
Contributor

Sort of as an additional issue, I wonder if this is something we can catch at compile time perhaps?

NathanSWard
NathanSWard approved these changes Jun 29, 2021
View reviewed changes
Copy link
Contributor

@NathanSWard NathanSWard 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 (other than my small nit).
Though, as I stated before, I'm curious if this is something we could potentially catch at compile time rather than runtime...

crates/bevy_ecs/src/system/system_param.rs Outdated Show resolved Hide resolved
@alice-i-cecile
Copy link
Member

Though, as I stated before, I'm curious if this is something we could potentially catch at compile time rather than runtime...

IIRC doing so is quite challenging, and will become increasingly so if we get dynamic systems or especially archetype invariants (#1481). I do sympathize with the desire though.

alice-i-cecile
alice-i-cecile approved these changes Jun 29, 2021
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is a clear uncontroversial improvement to an important error message.

@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 Jun 29, 2021
@alice-i-cecile
Copy link
Member

@mockersf shouldn't you just bors r+ this at this point instead of approving? Simple and useful error message fixes feel like they're within the "uncontroversial" scope :p

@mockersf
Copy link
Member

mockersf commented Jul 1, 2021

bors r+

bors bot pushed a commit that referenced this pull request Jul 1, 2021
@MinerSebas
Mention creation of disjoint Querys with Without<T> in conflicting …
b911a00 
…access Panic (#2413)

# Objective

Beginners semi-regularly appear on the Discord asking for help with using `QuerySet` when they have a system with conflicting data access.
This happens because the Resulting Panic message only mentions `QuerySet` as a solution, even if in most cases `Without<T>` was enough to solve the problem.

## Solution

Mention the usage of `Without<T>` to create disjoint queries as an alternative to `QuerySet`

## Open Questions

- Is `disjoint` a too technical/mathematical word?
- Should `Without<T>` be mentioned before or after `QuerySet`?
  - Before: Using `Without<T>` should be preferred and mentioning it first reinforces this for a reader.
  - After: The Panics can be very long and a Reader could skip to end and only see the `QuerySet`


Co-authored-by: MinerSebas <[email protected]>
@bors
Copy link
Contributor

bors bot commented Jul 1, 2021

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Mention creation of disjoint Querys with Without<T> in conflicting access Panic [Merged by Bors] - Mention creation of disjoint Querys with Without<T> in conflicting access Panic Jul 1, 2021
@bors bors bot closed this Jul 1, 2021
@MinerSebas MinerSebas deleted the disjoint_mention branch July 2, 2021 12:33
ostwilkens pushed a commit to ostwilkens/bevy that referenced this pull request Jul 27, 2021
@MinerSebas @ostwilkens
Mention creation of disjoint Querys with Without<T> in conflicting …
6c2e379 
…access Panic (bevyengine#2413)

# Objective

Beginners semi-regularly appear on the Discord asking for help with using `QuerySet` when they have a system with conflicting data access.
This happens because the Resulting Panic message only mentions `QuerySet` as a solution, even if in most cases `Without<T>` was enough to solve the problem.

## Solution

Mention the usage of `Without<T>` to create disjoint queries as an alternative to `QuerySet`

## Open Questions

- Is `disjoint` a too technical/mathematical word?
- Should `Without<T>` be mentioned before or after `QuerySet`?
  - Before: Using `Without<T>` should be preferred and mentioning it first reinforces this for a reader.
  - After: The Panics can be very long and a Reader could skip to end and only see the `QuerySet`


Co-authored-by: MinerSebas <[email protected]>
@MinerSebas MinerSebas mentioned this pull request Sep 2, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NathanSWard NathanSWard NathanSWard approved these changes

@mockersf mockersf mockersf approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MinerSebas @NathanSWard @alice-i-cecile @mockersf