#54 - CNV off latest master #887
Conversation
snpdb/models/models_variant.py
Outdated
| ref: str | ||
| alt: str | ||
|
|
||
| def __init__(self, chrom, start, end: int = None, ref: str = None, alt: str = None): |
There was a problem hiding this comment.
To make the typehint happy it should be
end: Optional[int] = None, ref: Optional[str] = None, alt: Optional[str] = None
as None is not a valid int or str
|
@TheMadBug - I made changes after your 1st review, if you could go over them again, thanks! |
| if full_match := VARIANT_PATTERN.fullmatch(clean_str): | ||
| def from_string(variant_string: str, regex_pattern=VARIANT_PATTERN): | ||
| # This will only ever match standard (non-symbolic variants) | ||
| if full_match := regex_pattern.fullmatch(variant_string): |
There was a problem hiding this comment.
Not that it did it before, but should this thow an exception if the string doesn't match the pattern?
Most of the code that calls VariantCoordinate.from_string() expects that there will be a value returned and if not, the code will generally do undesirable things like put Nones into lists etc.
There was a problem hiding this comment.
yep, better safe than sorry
snpdb/models/models_variant.py
Outdated
| @@ -254,21 +254,6 @@ class VariantCoordinate(FormerTuple): | |||
| ref: str | |||
| alt: str | |||
|
|
|||
| def __init__(self, chrom, start, end: int = None, ref: str = None, alt: str = None): | |||
There was a problem hiding this comment.
Though TypeHints might be able to pickup on some bad usage of VariantCoordinate, it's common enough that maybe validation in a post_init(): method might be a good start into "fail early" design.
There was a problem hiding this comment.
Added simple type checking (not sure what else you want - or whether we can do this automatically off type hints
There was a problem hiding this comment.
Re off type hints, found this seems designed specifically to ensure a dataclass has all the correct values
https://pypi.org/project/enforce-typing/
(we can introduce that as work after the merge to stop holding the merge back if you'd prefer)
| # Below this size, variants are stored with ref/alt sequences. Above this threshold, they become | ||
| # structural variants and use symbolic | ||
| VARIANT_SYMBOLIC_ALT_SIZE = 1000 | ||
| VARIANT_SYMBOLIC_ALT_VALID_TYPES = {"<DEL>", "<DUP>", "<INS>", "<CNV>"} |
There was a problem hiding this comment.
Not the biggest deal, but should all these be constants as they're referred a few times throughout.
They're small and standard enough that I'm not overly concerned if you'd prefer to leave them as strings.
There was a problem hiding this comment.
ok, did this just running tests
James to take a look before commit