Rewrite transformers as extension methods #83
Conversation
The usage is much nicer as extension methods. In most cases a small bit of code is copied for the replacement, though in some cases nearly the entire implementation is copied. This was done over extracting shared pieces so that the deprecated methods can be removed outright in the next major version change. Update and clean up some doc comments in the extension method version of these functions. Migrate tests to use the extension method versions. The old copies are now untested but they won't be modified before they are removed. Since there aren't useful needs within this package for `chainTransformers` or `map` they are deprecated without a replacement.
|
We won't be able to move forward with this until we have at least a dev SDK with extensions enabled. We likely won't publish until it's a stable SDK. I wanted to get the PR out early for feedback. |
The extension name is a publicly visible artifact and changing it will be breaking. It probably makes sense to combine these now.
|
One open question I have is how to think about which ones go into the same extension and how to name the extensions. It'll be breaking to change this later because of I don't think it makes sense to try to merge these all into one extension, so we'll want to consider carefully how to group them. |
There was a problem hiding this comment.
I didn't look at the impls - I assume they are the same.
Usage sites look great, clear improvement there that justifies the change imo.
As far as one extension versus multiple, I can't think of a reason to really prefer one over the other, except that organizationally within this package it seems nicer to have more targeted, smaller libraries. So I am perfectly fine with how it is currently set up.
lib/src/audit.dart
Outdated
| /// Returns a Stream which only emits once per [duration], at the end of the | ||
| /// period. | ||
| /// | ||
| /// Always introduces a delay of at most [duration]. |
There was a problem hiding this comment.
It's really at least not at most? Other async/sync work can starve this for an arbitrary amount of time.
There was a problem hiding this comment.
I think it's tricky to describe. "Always introduces a delay of at most [duration] plus arbitrary delay for unrelated async work." might be the most accurate.
Some events won't be emitted. For the events that are emitted, if it was the event that triggered the timer it will be delayed by duration + arbitrary_async. If it was an event that came in within that period it's delayed by (duration - delta_since_event_triggering_timer) + arbitrary_async...
There was a problem hiding this comment.
I updated the comment with a longer explanation and a timeline example. Does it make more sense this way?
…orm#83) The usage is much nicer as extension methods. In most cases a small bit of code is copied for the replacement, though in some cases nearly the entire implementation is copied. This was done over extracting shared pieces so that the deprecated methods can be removed outright in the next major version change. Update and clean up some doc comments in the extension method version of these functions. Migrate tests to use the extension method versions. The old copies are now untested but they won't be modified before they are removed. Since there aren't useful needs within this package for `chainTransformers` or `map` they are deprecated without a replacement.
The usage is much nicer as extension methods.
In most cases a small bit of code is copied for the replacement, though
in some cases nearly the entire implementation is copied. This was done
over extracting shared pieces so that the deprecated methods can be
removed outright in the next major version change.
Update and clean up some doc comments in the extension method version of
these functions.
Migrate tests to use the extension method versions. The old copies are
now untested but they won't be modified before they are removed.
Since there aren't useful needs within this package for
chainTransformersormapthey are deprecated without a replacement.