RecordStream icon indicating copy to clipboard operation
RecordStream copied to clipboard

20150520-multiplex-helpers

Open amling opened this issue 9 years ago • 7 comments

06a770ced6cc4efd529ccb1f4e19ade6f77d158e Rank and percentile for sort. edead1d482e57ef4e9414fdc1ccdf526c9410dd4 recs-linearextrapolate 208788e753af0082ab377ddecca81a7d8758846c Extract simple multiplex helper. 69751d362cd58b962879d768a540afa6866e6daf recs-zscore

Review on Reviewable

amling avatar May 21 '15 04:05 amling

Code looks good! Would you be up for writing some basic tests of zscore, linearextrapolate, and the new rank/percentile sort functionality?

I know RecordStream doesn't currently use roles for class composition, but SimpleMultiplexHelper really seems like something that's very suitable as a role. Instead of subclassing Operation, the consuming class would do that and the SimpleMultiplexHelper role would require the consuming class to implement the private _get_aggregators and _annotate_record methods. Role::Tiny would be a good role implementation for RecordStream.

tsibley avatar Jun 09 '15 16:06 tsibley

FYI, just leaving a note here that @amling is on vacation for about a month, so this will probably lie fallow for a while, totally cool

On Tue, Jun 9, 2015 at 9:41 AM Thomas Sibley [email protected] wrote:

Code looks good! Would you be up for writing some basic tests of zscore, linearextrapolate, and the new rank/percentile sort functionality?

I know RecordStream doesn't currently use roles for class composition, but SimpleMultiplexHelper really seems like something that's very suitable as a role. Instead of subclassing Operation, the consuming class would do that and the SimpleMultiplexHelper role would require the consuming class to implement the private _get_aggregators and _annotate_record methods. Role::Tiny would be a good role implementation for RecordStream.

— Reply to this email directly or view it on GitHub https://github.com/benbernard/RecordStream/pull/61#issuecomment-110426693 .

benbernard avatar Jun 09 '15 17:06 benbernard

Thanks for the heads up. :-)

tsibley avatar Jun 09 '15 17:06 tsibley

Oh, one more thing I wanted to note was that I think new recs-* symlinks should be dropped going forward. The recs command knows how to dispatch to operations without the symlinks, regardless of if it's a standalone version or the CPAN version.

tsibley avatar Jun 09 '15 17:06 tsibley

I'm fine with that, we sorta need to go back I think in that case and fix all the examples and also the documentations (like the recs story and recs examples

On Tue, Jun 9, 2015 at 10:12 AM Thomas Sibley [email protected] wrote:

Oh, one more thing I wanted to note was that I think new recs-* symlinks should be dropped going forward. The recs command knows how to dispatch to operations without the symlinks, regardless of if it's a standalone version or the CPAN version.

— Reply to this email directly or view it on GitHub https://github.com/benbernard/RecordStream/pull/61#issuecomment-110436953 .

benbernard avatar Jun 09 '15 17:06 benbernard

A big doc overhaul/cleanup is on my list for "sometime in the future." I'm not proposing we remove the current recs-* symlinks, just not add new ones going forward. New operations can start using the recs command invocations in their docs.

tsibley avatar Jun 09 '15 17:06 tsibley

nod makes sense to me

On Tue, Jun 9, 2015 at 10:31 AM Thomas Sibley [email protected] wrote:

A big doc overhaul/cleanup is on my list for "sometime in the future." I'm not proposing we remove the current recs-* symlinks, just not add new ones going forward. New operations can start using the recs command invocations in their docs.

— Reply to this email directly or view it on GitHub https://github.com/benbernard/RecordStream/pull/61#issuecomment-110441205 .

benbernard avatar Jun 09 '15 17:06 benbernard