-
Notifications
You must be signed in to change notification settings - Fork 41
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
ENH: Adding in differential type #212
Conversation
Ok - I just tested this against q2-aldex2 and it seems like the types are working. Not sure what is going with coveralls. Thoughts @nbokulich @ebolyen ? |
@mortonjt looks like you are not testing your new transformers |
clearly got too excited when I asked that question 🤣 Transformer + format tests are pushed in. Let's see if those coverall issues go away. |
Alright - copy pasta is done. Thanks @thermokarst ! |
@thermokarst / @nbokulich , will you be able to take a look at this PR? There are currently 4 different plugins that are relying on this type, namely biocore/songbird#51 It'll be really exciting to have this in! |
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.
A few high-level questions and comments. Sorry this didn't get reviewed sooner, I thought for some reason you had asked us out-of-band to hold off on reviewing --- my mistake.
Looks like tests are passing! |
Looks like the merge branch destroyed some of the tests from before grrr. Adding those back in. |
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.
LGTM, two minor question/comments inline. Thanks!
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.
The type name and format look very reasonable to me! I like that this is useful for multiple plugins, that'll be awesome.
(Matt obviously did the real review here, just piping in which my 2 cents)
Co-Authored-By: Matthew Dillon <matthewrdillon@gmail.com>
@thermokarst - all changes have been addressed. However, it appears that build has stalled ... |
Looks good to me. Thanks!!
…On Wed, Jun 19, 2019, 11:56 AM Matthew Dillon ***@***.***> wrote:
Thanks @mortonjt <https://github.com/mortonjt> - I added a few minor
tweaks in e41053f
<e41053f>,
if this looks okay to you just give this a thumbs-up --- I will merge once
you sign-off. Thanks!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#212?email_source=notifications&email_token=AA75VXKT34BVCIGQ5MKTYOTP3JJKHA5CNFSM4HD6PYOKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYCKO3Y#issuecomment-503621487>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA75VXMGBK6FUTWWFPK6Y33P3JJKHANCNFSM4HD6PYOA>
.
|
Thank you!
…On Wed, Jun 19, 2019, 4:32 PM Matthew Dillon ***@***.***> wrote:
Merged #212 <#212> into master.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#212?email_source=notifications&email_token=AA75VXJQ7OUMLEY5VTAYT2TP3KJVXA5CNFSM4HD6PYOKYY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOSCIRO5A#event-2425427828>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA75VXO6MQTZRDRXMANIC5TP3KJVXANCNFSM4HD6PYOA>
.
|
This addresses #211 and is still work in progress.
Note that this is a working progress and shouldn't be merged in until the corresponding PRs in songbird and q2-aldex2 have been created.