-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add support for theoretical and double antibodies in crossmatch api #1187
Add support for theoretical and double antibodies in crossmatch api #1187
Conversation
@kristinagalik davam se pryc, na prvni kolo prosim davat @abragtim a @jakubmonhart |
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.
Podle mě to funguje tak jak má, takže ok. Mám jenom pár poznámek/dotazů:
test_theoretical_and_double_antibodies_not_implemented
bych přejmenoval, protože double už je naimplementovany a theoretical antibody tady v žádném match nefiguruje (a tedy se tady ani netestuje).- Teď se tedy nikde netestuje jestli crossmatch api vrací správně theoretical antibody match že? (testuje se jenom že to správně dělá
get_crossmatched_antibodies_per_group
, tak je možná redundantní to testovat znovu jako výstup api když se theoretical match zpracovává stejně jako ostatní). - Je potřeba u všech těch parsing issue specifikovat v assert tu value? (viz třeba
ParsingIssueDetail.INSUFFICIENT_NUMBER_OF_ANTIBODIES_IN_HIGH_RES.value
) Funguje to stejně když dáš jenomParsingIssueDetail.INSUFFICIENT_NUMBER_OF_ANTIBODIES_IN_HIGH_RES
a možná je to lehce lépe čitelné (to je asi fakt detail, ale když už jsem si toho všiml tak to radši píšu :) ) - viz comment u kódu
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.
Už jenom jedna poznámka v kódu, jinak ok.
46daae2
to
74adb2d
Compare
Closes #1168