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

Prefer System Tuples, replace in_schedule, aggressive add_systems merging #10

Merged
merged 11 commits into from
Sep 19, 2022

Conversation

cart
Copy link

@cart cart commented Sep 17, 2022

Followup to #9 (which should be merged first)

This pr includes a number of small UX tweaks (some of which will be controversial), each split up into its own commit. I personally like where it landed, but every commit is negotiable. The last couple are especially controversial I think 😄

@cart cart force-pushed the prefer-macroless branch 2 times, most recently from 2c9526f to 50fa6f5 Compare September 17, 2022 03:10
Copy link
Owner

@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 like add_systems_to. The double-parantheses and tuple abuse are a bit naff, but we'll need a builder style escape hatch anyways to account for arbitrary lengths so people who prefer it can use that instead.

add_system should still exist, but I do like grouping systems by default.

Tuple nesting seems both natural and good.

Copy link
Collaborator

@maniwani maniwani left a comment

Choose a reason for hiding this comment

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

LGTM.

I'd prefer if each sentence was on its own line to be consistent with the rest of the doc (makes it easier for GitHub to diff changes).

@cart
Copy link
Author

cart commented Sep 17, 2022

The double-parantheses and tuple abuse are a bit naff, but we'll need a builder style escape hatch anyways to account for arbitrary lengths so people who prefer it can use that instead.

Cool cool. I made sure to call out Into<SystemCollection> to leave the door open to arbitrary builder apis.

add_system should still exist, but I do like grouping systems by default.

Yeah I think I'm on that team too (easier migration path, reads better for single systems). I intentionally took it "too far", just too see how it made us feel 😄

I'd prefer if each sentence was on its own line to be consistent with the rest of the doc (makes it easier for GitHub to diff changes).

I haven't sorted out my feelings on this style, but I'm happy to align with it here for consistency.

@maniwani
Copy link
Collaborator

maniwani commented Sep 18, 2022

I haven't sorted out my feelings on this style, but I'm happy to align with it here for consistency.

It should render the same. I just mean in the file.

If you have two sentences on different lines, the generated Markdown will still show them on the same line.

My first sentence.
My second sentence.

My first sentence. My second sentence.

They would only be rendered on different lines when there was an additional newline in between them, or if there were two spaces after the first sentence. Not what I wanted.

Having each sentence on a different line of the file has made it a little easier for us to view the changes in the diff view. Though I guess if you're going to merge the RFC and we never edit this document again, then it doesn't really matter.

@cart
Copy link
Author

cart commented Sep 19, 2022

It should render the same. I just mean in the file.

Yeah I'm not worried about that. My only hangup is "forcing people into this style as a matter of policy". Idk if the churn there is worth the diff-reviewability wins. But thats a conversation we should have later. Happy to align with the style in the current RFC. Consistency is the only reason I need.

Copy link

@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.

Renaming SystemLabel -> SystemSet will clear up a lot of potential confusion, I think. Removing some macros is nice, too.

@alice-i-cecile alice-i-cecile merged commit d12e5c1 into alice-i-cecile:stageless Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants