microsoft / datamations

https://microsoft.github.io/datamations/
Other
66 stars 14 forks source link

Added functionality for the tally verb #136

Closed willdebras closed 2 years ago

willdebras commented 2 years ago

The commits add support for the tally verb by doing the following:

ghost commented 2 years ago

CLA assistant check
All CLA requirements met.

willdebras commented 2 years ago

@jhofman Wanted to get a quick verify I should go ahead and merge this. You mentioned AOK to merge in our last meeting, but wanted to verify I should be making merges and check in about review/QC process.

jhofman commented 2 years ago

@willdebras, yes all good in terms of review process.

one late-occurring thought on this: is it in any way safer or easier to just map the "tally" function to prep_specs_count, since it's just an alias? could cut down on some redundant code? (hadn't thought of that until now.)

willdebras commented 2 years ago

I've thought a bit about if we want to integrate tally in a way that just aliases prep_specs_count, but I think it makes sense to leave them as two separate files. Since prep_specs_count is returning to-be-json response, I don't want to retroactively update that response with the custom animation metadata or the title metadata and I think it would muddy code a bit to add additional parameters in the do.call on the call_verb in datamation_sanddance() to get the specification for these. I think pretty clean to just have a separate file for each. I'm going to go ahead and merge.