LooseLab / readfish

CLI tool for flexible and fast adaptive sampling on ONT sequencers
https://looselab.github.io/readfish/
GNU General Public License v3.0
169 stars 33 forks source link

Feature/check minknow #351

Closed mattloose closed 5 months ago

mattloose commented 5 months ago

This pull request is to address #349 and is raised for discussiong.

Adoni5 commented 5 months ago

i love a good discussiong

Adoni5 commented 5 months ago

I'm going to change the target for this to feature/dorado

mattloose commented 5 months ago

i love a good discussiong

discussiong: noun the action or process of talking about something in order to reach a conclusion which may take a very long time.

mattloose commented 5 months ago

I'm going to change the target for this to feature/dorado

sorry - my bad - yep please do.

Adoni5 commented 5 months ago

And I think you need to add packaging as a dependency!

Adoni5 commented 5 months ago

And I think you need to add packaging as a dependency!

I see that it is a dependency of the minknow_api, but maybe we could explicitly add it as a dependency anyway? Explicit is better than implicit after all

mattloose commented 5 months ago

I've moved this to merge into the feature/dorado branch - as it has all the commits from that branch as part of it's history!

Is that correct?

probably

Adoni5 commented 5 months ago
* the `_compatibility.DIRECTION` enum is a little opaque on first reading. It's not clear if that's saying upgrade readfish or MinKNOW.

https://github.com/LooseLab/readfish/blob/dc6644cef7bf9acfcba3889fa5b7490b23c7240a/src/readfish/_compatibility.py#L29-L31

Amazing stuff, I don't know how it could be clearer 👾 ( Definitely thought that this docstring actually had contents)

* `check_compatibility` looks hard to use in the code. Does it need to return a tuple/could the check be specifically for an actionable `DIRECTION` variant? E.g:
if (action := check_compat(...)) in (DIRECTION.UP, DIRECTION.DOWN):
    log(action)

Yeah agreed, it should just return the Enum variant or None not the tuple, makes more sense 👍🏼

alexomics commented 5 months ago

LGTM! 🚀

mattloose commented 5 months ago

Much awesomeness.