search

cthoyt / chembl-downloader

Write reproducible code for getting and processing ChEMBL
https://chembl-downloader.readthedocs.io
MIT License
65 stars 11 forks source link

Added max_phase as an option #13

Closed jhylin closed 6 months ago

jhylin commented 6 months ago

This PR is related to this issue #1, many thanks.

cthoyt commented 6 months ago

In your post, you just had it return the max phase. Here, it look like you're adding a filter. I think a different way to do this is to just add a boolean flag for returning max phase and then format it in to the SELECT part of the query instead of the WHERE part

cthoyt commented 6 months ago

@jhylin yes! I made a minor update in 72cd48a but this should do it. Did you test it works locally?

jhylin commented 6 months ago

Yes, I just did a unittest on the latest code (I've pushed the test_queries.py in the /test folder), and it worked. The only thing was (very likely due to my lack of experience in doing unittest), I had to temporarily make target_id as Optional[str], and the test passed - unsure how I would resolve the error message regarding "missing 1 positional statement for target_id" in the unittest if I didn't make it optional?

cthoyt commented 6 months ago

@jhylin the trick was that you were missing the target_id argument - you just had to pass a string. I did a bit of re-org and merged this. Thanks! Will make a release momentarily.