Impl From<Encoding> for Scene.
#538
Conversation
src/scene.rs
Outdated
| @@ -219,6 +223,17 @@ impl Scene { | |||
| } | |||
| } | |||
|
|
|||
| impl From<Encoding> for Scene { | |||
| fn from(encoding: Encoding) -> Self { | |||
| // TODO: Properly update the estimator for the encoding. | |||
There was a problem hiding this comment.
And this is why I'd make it a from_encoding method, to document that the bump estimator will undercount items added in this method
There was a problem hiding this comment.
In other conversations, it has already been said that the implementation strategy of the estimator will be changing and this issue would go away.
There was a problem hiding this comment.
Note that if you're adding a TODO to the code, it's usually good practice to join a link to a ticket (usually a github issue) tracking down whatever needs to be done to remove that TODO.
There was a problem hiding this comment.
It matches the existing TODO comments elsewhere in the file for the same feature. (Also, out of roughly 80 TODOs in this repo, approximately 2 have an issue number in them.)
There was a problem hiding this comment.
We can do a comments pass at some point, but I think at least new comments should follow that practice.
There was a problem hiding this comment.
We could point to #530 (comment)
|
If there's feedback that this functionality is desired, I'll think about updating and re-opening or someone else can run this gauntlet. |
This allows creating a `Scene` with a pre-existing `Encoding`. Fixes linebender#530.
waywardmonkeys
force-pushed
the
allow-creation-of-scene-from-encoding
branch
from
73abc10
to
24fdd3f
Compare
There was a problem hiding this comment.
Thanks - this looks like it should give more flexibility to consumers
The concern about bump estimation and the future state of that are valid, but we're in flux at the moment, and @armansito's comment in the linked issue is clear enough that the bump estimator field will change substantially
I don't think adding a link to that comment is too important, although it would probably be an improvement
src/scene.rs
Outdated
| @@ -219,6 +223,17 @@ impl Scene { | |||
| } | |||
| } | |||
|
|
|||
| impl From<Encoding> for Scene { | |||
| fn from(encoding: Encoding) -> Self { | |||
| // TODO: Properly update the estimator for the encoding. | |||
There was a problem hiding this comment.
We could point to #530 (comment)
|
Thanks! This is a good extension to our API |
This allows creating a
Scenewith a pre-existingEncoding.Fixes #530.