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] - [Fixes #6224] Add logging variants of system piping #6751

Closed
wants to merge 11 commits into from
Closed

[Merged by Bors] - [Fixes #6224] Add logging variants of system piping #6751

wants to merge 11 commits into from

Conversation

Edwox
Copy link
Contributor

@Edwox Edwox commented Nov 25, 2022

Objective

Fixes #6224, add dbg, info, warn and error system piping adapter variants to expand #5776, which call the corresponding re-exported bevy_log macros when the result is an error.

Solution

  • Added dbg, info, warn and error system piping adapter variants to system_piping.rs.
  • Modified and added tests for these under examples in system_piping.rs.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use labels Nov 25, 2022
alice-i-cecile
alice-i-cecile requested changes Nov 25, 2022
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 am so keen to get these added to Bevy itself: this pattern is incredibly useful.

I've left some suggestions for better documentation.

@Edwox
Copy link
Contributor Author

Edwox commented Nov 25, 2022

I am so keen to get these added to Bevy itself: this pattern is incredibly useful.

I've left some suggestions for better documentation.

Great! I will try to implement these suggestions right away.

@harudagondi
Copy link
Member

Please run cargo fmt --all =)

@alice-i-cecile
Copy link
Member

Once you've fixed up reviewers' comments, please resolve the conversation so others can see what still needs to be fixed :)

@alice-i-cecile alice-i-cecile added this to the 0.10 milestone Nov 26, 2022
JoJoJet
JoJoJet suggested changes Nov 26, 2022
View reviewed changes
crates/bevy_ecs/src/system/system_piping.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/system_piping.rs Outdated Show resolved Hide resolved
@Edwox Edwox marked this pull request as ready for review December 8, 2022 20:05
JoJoJet
JoJoJet approved these changes Dec 8, 2022
View reviewed changes
Copy link
Member

@JoJoJet JoJoJet left a comment

Choose a reason for hiding this comment

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

Very nice.

In the future, I think we should look into ways of including the input system's name in these error messages.

@Edwox
Copy link
Contributor Author

Edwox commented Dec 9, 2022 •
edited

Very nice.

In the future, I think we should look into ways of including the input system's name in these error messages.

Yeah, I agree! I thought about implementing something like that.

alice-i-cecile
alice-i-cecile approved these changes Dec 10, 2022
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.

This PR is so nice. I'm looking forward to using these aggresively.

@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 Dec 10, 2022
@alice-i-cecile
Copy link
Member

@Edwox if you get a chance before I merge this, can you add these adaptors to the bevy_ecs prelude? These seem widely useful.

@Edwox
Copy link
Contributor Author

Edwox commented Dec 10, 2022

@Edwox if you get a chance before I merge this, can you add these adaptors to the bevy_ecs prelude? These seem widely useful.

Sure, I'll get right on it!

@Edwox
Added adapters unwrap, dbg, info, warn, error, a…
f353527 
…nd ``ignore`` to the ``bevy_ecs::prelude``
@alice-i-cecile
Copy link
Member

@Edwox Once CI is passing I'll merge this. Please ping me if I forget!

@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Dec 11, 2022
@Edwox
[Fixes #6224] Add logging variants of system piping (#6751)
aea4c5b 
# Objective

Fixes #6224, add ``dbg``, ``info``, ``warn`` and ``error`` system piping adapter variants to expand #5776, which call the corresponding re-exported [bevy_log macros](https://docs.rs/bevy/latest/bevy/log/macro.info.html) when the result is an error.

## Solution

* Added ``dbg``, ``info``, ``warn`` and ``error`` system piping adapter variants to ``system_piping.rs``. 
* Modified and added tests for these under examples in ``system_piping.rs``.
@bors
Copy link
Contributor

bors bot commented Dec 11, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title [Fixes #6224] Add logging variants of system piping [Merged by Bors] - [Fixes #6224] Add logging variants of system piping Dec 11, 2022
@bors bors bot closed this Dec 11, 2022
@Edwox Edwox deleted the bevy_add_logging_variants_of_system_chaining branch December 11, 2022 18:22
@alice-i-cecile alice-i-cecile mentioned this pull request Jan 15, 2023
Open
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
@Edwox @alradish
[Fixes bevyengine#6224] Add logging variants of system piping (bevyen…
1bad991 
…gine#6751)

# Objective

Fixes bevyengine#6224, add ``dbg``, ``info``, ``warn`` and ``error`` system piping adapter variants to expand bevyengine#5776, which call the corresponding re-exported [bevy_log macros](https://docs.rs/bevy/latest/bevy/log/macro.info.html) when the result is an error.

## Solution

* Added ``dbg``, ``info``, ``warn`` and ``error`` system piping adapter variants to ``system_piping.rs``. 
* Modified and added tests for these under examples in ``system_piping.rs``.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@Edwox @ItsDoot
[Fixes bevyengine#6224] Add logging variants of system piping (bevyen…
57ae416 
…gine#6751)

# Objective

Fixes bevyengine#6224, add ``dbg``, ``info``, ``warn`` and ``error`` system piping adapter variants to expand bevyengine#5776, which call the corresponding re-exported [bevy_log macros](https://docs.rs/bevy/latest/bevy/log/macro.info.html) when the result is an error.

## Solution

* Added ``dbg``, ``info``, ``warn`` and ``error`` system piping adapter variants to ``system_piping.rs``. 
* Modified and added tests for these under examples in ``system_piping.rs``.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harudagondi harudagondi harudagondi left review comments

@LinusKall LinusKall LinusKall left review comments

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

@JoJoJet JoJoJet JoJoJet approved these changes

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

Successfully merging this pull request may close these issues.

Add logging variants of system chaining / piping adaptors
5 participants
@Edwox @harudagondi @alice-i-cecile @JoJoJet @LinusKall