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

remove inaccurate warning from in_state #13862

Merged
merged 3 commits into from
Jun 16, 2024
Merged

remove inaccurate warning from in_state #13862

merged 3 commits into from
Jun 16, 2024

Conversation

lee-orr
Copy link
Contributor

@lee-orr lee-orr commented Jun 15, 2024

Objective

Fixes #13854

Solution

Removed the inaccurate warning. This was done for a few reasons:

  • States not existing is now a valid "state" (for lack of a better term)
  • Other run conditions don't provide an equivalent warning

@lee-orr
remove inaccurate warning from in_state
0ddc729 
Reasoning:
- other state-related run conditions do not have an equivalent warning
- None is a now a valid value for the `State` resource
@jgayfer jgayfer added A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior labels Jun 15, 2024
jgayfer
jgayfer approved these changes Jun 15, 2024
View reviewed changes
Copy link
Contributor

@jgayfer jgayfer left a comment

Choose a reason for hiding this comment

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

Looks like we might need a cargo fmt.

First time reviewing an ECS PR, but overall this makes sense to me given the issue at hand.

@jgayfer jgayfer added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Jun 15, 2024
@alice-i-cecile alice-i-cecile added this to the 0.14 milestone Jun 15, 2024
alice-i-cecile
alice-i-cecile approved these changes Jun 15, 2024
View reviewed changes
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 think this is correct to remove overall. If users express confusion in the future we can think harder about a targeted warning.

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 15, 2024
MiniaczQ
MiniaczQ approved these changes Jun 15, 2024
View reviewed changes
Copy link
Contributor

@MiniaczQ MiniaczQ left a comment

Choose a reason for hiding this comment

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

Agreeing with predecessors

@alice-i-cecile
Copy link
Member

@lee-orr once CI is green I'll merge this. It's a simple unused item problem.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 16, 2024
Merged via the queue into bevyengine:main with commit f691173 Jun 16, 2024
27 checks passed
mockersf pushed a commit that referenced this pull request Jun 19, 2024
@lee-orr @mockersf
remove inaccurate warning from in_state (#13862)
1c838ff 
# Objective
Fixes #13854

## Solution
Removed the inaccurate warning. This was done for a few reasons:

- States not existing is now a valid "state" (for lack of a better term)
- Other run conditions don't provide an equivalent warning
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@jgayfer jgayfer jgayfer approved these changes

@MiniaczQ MiniaczQ MiniaczQ approved these changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior 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
0.14
Development

Successfully merging this pull request may close these issues.

Incorrect WARN when using sub states
4 participants
@lee-orr @alice-i-cecile @jgayfer @MiniaczQ