Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Merged by Bors] - Auto-label function systems with SystemTypeIdLabel #4224
[Merged by Bors] - Auto-label function systems with SystemTypeIdLabel #4224
Changes from 4 commits
e8a5c6a
649987b
76ca7f6
a964e8e
ca7c674
c39276e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or if we can afford it, remove the
pub
entirely.Like #4250. This is more of an implementation detail than anything else, probably shouldn't made public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imo this label is a part of our public api (especially with my recent changes, which remove the box from AsSystemLabel). I think there might be cases where this should be accessible to users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a short doc comment stating this is used for system -> SystemLabel coercion plus a pointer to SystemLabel which illustrates the ergonomics.
It'd also stave off the perpetually missing
#[forbid(missing_docs)]
for the crate.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this to be a method, for similar reasons we don't need the analagous
IntoSystem::into_system
as a method.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idk if "non method Self functions" is a pattern I want to encourage generally. We had a good reason to do that for IntoSystem: deprecating .system() let us "encourage" users to use the more ergonomic pattern without outright breaking their code. We don't have that context here. Given that methods are the more common pattern, I think the more relevant question is: why not make this a method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly it's never meant to be used by end users, but will clutter up the flyimport suggests when users are attempting to find
.before
or.after
, especially as alphabetically it is in between those two options.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cluttering auto-complete imports is actually a compelling argument here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I agree, given that getting the "closure" label is something you'd want to do with an
as_system_label
call (which would benefit from method-style syntax). Other use cases may emerge in the future too (ex: if something needs to accept a list of labels, we can't pass a bunch of different AsSystemLabel impls ... we need to convert to boxed labels before the call).I personally don't think autocomplete is much of an issue here, as I think most people type at least the first letter of what they're looking for in autocomplete.
.b
for.before
,.a
for.after
....after
comes before.as
in an alphabetically sorted list.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The need to clone here is an unfortunate 'regression'
Is there any downside to just making
AsSystemLabel
consume self?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consuming
self
would make this unusable for closure systems, as retrieving the label would consume the IntoSystem impl (aka the closure).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this doesn't add extra boxing (this replaces the manual boxing that we did in the past). Given that basically all system labels are enums, typeids, or ZSTs, the extra clone probably wouldn't even be measurable, even for many thousands of systems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's already hugely unergonomic for closure systems though. Either you create the 'same' closure twice:
Or you store it in a local variable:
But yeah, perhaps being able to use the second pattern is worthwhile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this is so challenging (and niche) for closure systems that we shouldn't go out of our way to support it. Providing an explicit label is a nice clear workaround.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a really reasonable use case / pattern for closure systems. Given that closure systems are now "the" way to pass in-scope configuration into a system, and closures will also exist in that scope, I think its reasonable to support ordering without needing to define custom labels. At the very least, I think this should be possible (and it currently is):
But I'm also open to making @DJMcNab's second suggestion work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just made AsSystemLabel return the actual label type instead of a box. This doesn't "resolve" the extra clone, but it does feel like a cleaner design to me. I really don't think this should consume self, as I don't think we should commit to all system instances being "disposable". "closure systems" already aren't disposable, and things like the current manual FixedTimestep system implementation also "hold state" and shouldn't be disposed of here (I know that the FixedTimestep impl is controversial, but its illustrative of the "custom system" pattern). I stand by the thought that an extra clone here won't hurt anyone, especially when labels are basically exclusively ZSTs and enums.