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
Make a dynamically applicable version of
Bundle
#3694Make a dynamically applicable version of
Bundle
#3694Changes from 1 commit
c0e7fc9
4a13a80
0f2cbdd
57caadc
8cfd009
8322460
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.
Maybe call this
DynBundle
?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.
Possibly? It is not limited to dynamic use cases; just cases where
self
is available. This is the main entry way to 'single bundle insertion'.Normal
Bundle
is absolutely static, but that doesn't mean thatApplicableBundle
is not static.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.
My preference here is either:
Bundle
, and useStaticBundle
for the other trait.InsertableBundle
orCreationBundle
.The former is nice, because in 99% of cases, users only need the methods from this trait to work with what they need. Removing bundles is typically not very useful. However it's a bit confusing, as deriving Bundle would derive a trait and its super trait.
The latter is just playing with synonyms to better reflect intent in terms of things users are familiar with.
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 did consider option 1. My concern was that it requires any manual impls of
Bundle
to know that it's actually a different trait which gets implemented. This change is currently non-breaking (as far as I can tell), which is quite neat.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 I think that manual impls of
Bundle
are going to be exceedingly rare, and the compiler errors should be quite useful. The fact thatBundle
would now be unsafe would also encourage people to Read the Docs.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.
?
Bundle
has always beenunsafe
. I do agree though that manual impls are going to be rare.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.
One other thing is that the renaming would require a careful audit of documentation etc. as
Bundle
can no longer be used for removal operations. I don't really want to do that in this PR if it can be avoided.