[DNM] asap-data Fix empty ligand to_oemol() error
#998
Conversation
| def test_empty_ligand(): | ||
| lig = Ligand.from_smiles("", compound_name="test_empty") | ||
| assert lig.data is None | ||
| assert lig.to_oemol() is None |
There was a problem hiding this comment.
Am I wrong in thinking that the Ligand instance shouldn't be able to return a None for this method? This is something that we commonly want to avoid, because this can easily lead to the infamously common "NoneType is not xxxxxx" error or similar further along the pipeline, which we want to avoid. We want to fail "early".
There was a problem hiding this comment.
Or in other words, this looks like working around a deeper problem in the underlying classes, namely Ligand in this case.
There was a problem hiding this comment.
Agreed, what do you think we should be returning instead? or set a validation error for Ligand if it is empty?
There was a problem hiding this comment.
yes, in my opinion having it to fail when it's None would be better, but that would need to be tested since the behavior is already there and some tools further down the pipeline might already be using this. Now, to be fair, according to the documentation it is not supposed to return None, and that's the contract we are willing to sign. So, if we are following the documentation (and type annotations) this should be "fine".
hmacdope
changed the title
asap-data Fix empty ligand to_oemol() errorasap-data Fix empty ligand to_oemol() error
Description
fixes #990
Todos
Notable points that this PR has either accomplished or will accomplish.
Questions
Status
Developers certificate of origin