ImagingDataCommons / idc-index

Python package to simplify access to the data available from NCI Imaging Data Commons
https://idc-index.readthedocs.io/
MIT License
11 stars 5 forks source link

BUG: fixed sql_query function to work with multiple indices #114

Closed DanielaSchacherer closed 2 months ago

DanielaSchacherer commented 2 months ago
fedorov commented 2 months ago

# TODO: find a more elegant way to automate the following

AI to the rescue: https://www.perplexity.ai/search/write-python-code-that-iterate-XY9ppywbQFSRnOpgbwx_uQ (I have not tested the code, definitely needs to be tested!).

fedorov commented 2 months ago

The AI-suggested part is a bit controversial, but I don't know if there is a better alternative. This probably needs more research and exploration. It does not need to be part of this PR, can keep in TODO, maybe include a link to this PR in the code so it can be followed up later.

DanielaSchacherer commented 2 months ago

AI to the rescue: https://www.perplexity.ai/search/write-python-code-that-iterate-XY9ppywbQFSRnOpgbwx_uQ (I have not tested the code, definitely needs to be tested!).

Yes, I asked another AI and got a similar suggestion with a strong recommendation to not do this in production code. That's why I added the comment and kept the code as it was.

DanielaSchacherer commented 2 months ago
  • might be good to add a test!

If it's okay, I would like to keep that for the next PR when we automate this process.

fedorov commented 2 months ago

Looks good, but do you think you could squash commits into a single one and force-push?

$ git rebase -i HEAD~3
fedorov commented 2 months ago

Sorry for nit-picking, but can you do git commit --amend and remove the ENH lines? I do not think they add any value. You could have also removed those as part of the rebase operation.

I should update contribution instructions to add those details.

DanielaSchacherer commented 2 months ago

I wasn't aware those lines were still there. They should be gone now!