Skip to content

[Merged by Bors] - Remove last uses of string-labels #5420

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
Closed
wants to merge 9 commits into from
Closed

[Merged by Bors] - Remove last uses of string-labels #5420

wants to merge 9 commits into from

Conversation

joseph-gio
Copy link
Member

@joseph-gio joseph-gio commented Jul 22, 2022 •
edited
Loading

Objective

@joseph-gio joseph-gio force-pushed the string-label-tests branch from f94d708 to 29a1f04 Compare July 22, 2022 04:17
@joseph-gio joseph-gio marked this pull request as ready for review July 22, 2022 05:20
@joseph-gio joseph-gio changed the title Remove string-labels from tests Remove last uses of string-labels Jul 22, 2022
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change labels Jul 22, 2022
alice-i-cecile
alice-i-cecile approved these changes Jul 22, 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.

Looks good! Thanks, I was dreading fixing this.

@joseph-gio joseph-gio force-pushed the string-label-tests branch from 2f582b0 to f80e340 Compare July 24, 2022 18:41
tim-blackbird
tim-blackbird suggested changes Aug 9, 2022
View reviewed changes
Copy link
Contributor

@tim-blackbird tim-blackbird left a comment

Choose a reason for hiding this comment

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

I'm glad we're getting rid of string labels :)

I don't like those macros. They do the same thing as the derives and allow non camel-case structs, which does not spark joy :c

crates/bevy_ecs/README.md
@@ -148,9 +148,13 @@ fn main() {
// Create a new Schedule, which defines an execution strategy for Systems
let mut schedule = Schedule::default();

// Define a unique public name for a new Stage.
#[derive(StageLabel)]
pub struct UpdateLabel;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
pub struct UpdateLabel;
struct UpdateLabel;

@BorisBoutillier
Copy link
Contributor

Am i missing something, or the added macros are only to remove one line each time, the #[derive(XXXLabel) call ?
Meaning

run_criteria_label!(AlwaysNo);

is equivalent to

#[derive(RunCriteriaLabel)
struct AlwaysNo

If this is the case, I wonder if the macro are really that useful ?
For bevy's repository, there is more code in the macro themselves than in the reduced declaration lines.
And I don't think we want to publicize the macros to bevy users ?

@joseph-gio
Copy link
Member Author

Am i missing something, or the added macros are only to remove one line each time, the #[derive(XXXLabel) call ?

It allows you to define a bunch of labels on a single line with no more boilerplate than is necessary. I think we do need to have some way of effortlessly declaring private labels, since the SystemLabelTypeId trick wont work for other varieties of labels.

It might be more consistent to replace single-argument macro calls with a derive, though.

If this is the case, I wonder if the macro are really that useful ? For bevy's repository, there is more code in the macro themselves than in the reduced declaration lines.

I don't think we should be looking at the raw line count. These macros move the boilerplate code somewhere else, where it won't distract from the behavior that actually matters.

And I don't think we want to publicize the macros to bevy users?

If they're useful to us, then they'll be useful to users IMO.

@alice-i-cecile
Copy link
Member

I don't want to publicize these macros; it's not part of the public API that I want to commit to supporting and they're easy to recreate.

@joseph-gio
Copy link
Member Author

I just ended up removing the macros entirely. If they're only usable within bevy_ecs itself (no examples or docs), then there's really no way for them to carry their weight.

tim-blackbird
tim-blackbird approved these changes Aug 10, 2022
View reviewed changes
Copy link
Contributor

@tim-blackbird tim-blackbird 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 beside one little thing :)

And I like the enums. They're nice.

@joseph-gio
Copy link
Member Author

joseph-gio commented Aug 28, 2022 •
edited
Loading

Just fixed up some merge conflicts.

@rparrett rparrett 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 Sep 1, 2022
alice-i-cecile
alice-i-cecile approved these changes Sep 3, 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.

Nice work here; thanks for demacroifying the code as requested. Yeet!

bors r+

bors bot pushed a commit that referenced this pull request Sep 3, 2022
@joseph-gio
Remove last uses of string-labels (#5420)
697d297 
# Objective

* Related: #4341
* Remove all remaining uses of stringly-typed labels in the repo. Right now, it's just a bunch of tests and examples.
@bors
Copy link
Contributor

bors bot commented Sep 3, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Remove last uses of string-labels [Merged by Bors] - Remove last uses of string-labels Sep 3, 2022
@bors bors bot closed this Sep 3, 2022
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
@joseph-gio @james7132
Remove last uses of string-labels (bevyengine#5420)
b9ed8bd 
# Objective

* Related: bevyengine#4341
* Remove all remaining uses of stringly-typed labels in the repo. Right now, it's just a bunch of tests and examples.
@joseph-gio joseph-gio deleted the string-label-tests branch December 1, 2022 15:06
@joseph-gio joseph-gio restored the string-label-tests branch December 1, 2022 15:06
@joseph-gio joseph-gio deleted the string-label-tests branch December 1, 2022 15:06
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@joseph-gio @ItsDoot
Remove last uses of string-labels (bevyengine#5420)
67f3422 
# Objective

* Related: bevyengine#4341
* Remove all remaining uses of stringly-typed labels in the repo. Right now, it's just a bunch of tests and examples.
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

@BorisBoutillier BorisBoutillier BorisBoutillier approved these changes

@tim-blackbird tim-blackbird tim-blackbird approved these changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Scheduling
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@joseph-gio @BorisBoutillier @alice-i-cecile @tim-blackbird @rparrett