SACGF / SACGF/variantgrid

Split HGVSDisplay - extract HGVSComponents from the display wrapper

Open
#1,702 1 comment 0 reactions 1 assignee Claimed by @davmlaw View on GitHub
Dominant language
Python
Stars
30
Forks
3
Avg merge
9h 28m
Merged PRs (30d)
42

Description

🤖 Written by Claude

`HGVSDisplay` (`genes/hgvs/hgvs.py`, formerly `CHGVS`) has accumulated four unrelated jobs. It's constructed from a bare string at 43 non-test sites — mostly datatables, template tags and export rows reading DB columns — so it needs to stay a cheap, lenient, dependency-free value object. Today it isn't: it reaches into `genes.models`, runs DB queries, and mixes mutable view state into its ordering and equality.

The rename to `HGVSDisplay` (#1063 branch) named it after what most of it currently is. This issue is about extracting the part that isn't.

## Why it can't just become `HGVSVariant`

`HGVSVariant` is an ABC in the separate `hgvs_shim` package, and its only implementations wrap pyhgvs `HGVSName` / biocommons `SequenceVariant`. You can only get one via a converter, and biocommons' `create_hgvs_variant()` raises `HGVSNomenclatureException` on anything malformed.

`HGVSDisplay` is deliberately total — non-matching input falls through to `nomen`, and `HGVSDisplay("")` is a used idiom (`classification_tags.py:428`). It parses whatever is already in the database, including legacy junk. That leniency is a feature for a column renderer and a bug for a parser.

It also can't become free functions: `__eq__` / `__hash__` / `__lt__` / `sort_str` are what earn it its keep. `set[HGVSDisplay]` and `sorted({...})` are used in at least four places (`allele_overlaps.py:264`, `discordance_models.py:433`, `discordance_report_views.py:255`, `allele_grouping_datatables.py:90`).

## Two live correctness edges

These are the reason this is worth doing rather than just tidy:

1. **Sorting depends on a display flag.** `sort_str` prefixes `"A"` if `is_normalised` else `"Z"`, then appends `genome_build.pk`. `__lt__` uses `sort_str`, so every `sorted(...)` over these silently depends on whether the caller remembered to set a flag that has nothing to do with HGVS ordering.

2. **`__eq__` and `__hash__` disagree.** `__eq__` compares `full_c_hgvs` **and** `is_normalised` **and** `genome_build`; `__hash__` uses `full_c_hgvs` alone. Legal, but it means two objects can be unequal while colliding in a set, so `set()` membership varies with the order flags were assigned.

## Move out

**→ `genes/transcript_parts.py`** — `transcript_parts` regex-parses `NM_000059.3` into `TranscriptParts`. `get_transcript_id_and_version()` already does this by splitting on `.`, and `HGVSMatcher.get_transcript_parts()` uses that one. The two don't agree: the regex `^([_A-Z0-9]+)(?:[.]([0-9]+))?$` silently returns `TranscriptParts(None, None)` for an accession containing an unexpected character, while the split version accepts anything. Delegate to the existing function.

**→ `genes/transcripts_utils.py`** — `_clean_transcript`, LRG upper/lower normalisation via `LRGRefSeqGene`. Transcript-accession hygiene, not HGVS parsing; `transcript_is_lrg` already lives there.

**→ `TranscriptVersion` classmethod** — `transcript_version_model()` runs two DB queries. It is the class's only DB-touching member, has exactly one caller (`ResolvedVariantInfo.recalc_c_hgvs()`), and is single-handedly why the value object imports `genes.models`. A lookup keyed on `(identifier, version, genome_build)` belongs on the model.

**→ `hgvs_shim` or a module-level function** — `c_dot_equivalent` + `C_DOT_PARTS`, nucleotide equivalence (`c.2891del` vs `c.2891delC`). Pure stateless string logic about HGVS nomenclature, only consumer is `diff()`.

**→ its own module** — `PHGVS` shares the file but nothing else. It is also already the shape the core should be: frozen dataclass, `parse()` classmethod, no Django, no DB.

## Split what's left

After those moves, the remainder divides cleanly:

**`HGVSComponents`** — a lenient parser, an immutable string re-writer, and a comparator. No Django, no DB, no genome build, no display state. Roughly:

- parse `NM_000059.3(BRCA2):c.1234A>G` into `transcript` / `gene_symbol` / `nomen`, keeping the original string; never raises
- `without_gene_symbol_str`, `with_gene_symbol()`, `with_transcript_version()`, `without_transcript_version`
- `diff()` → `HGVSDiff` (rename `CHGVSDiff`/`chgvs_diff_description` with it)
- `__eq__` / `__hash__` / `__str__` on the string alone, plus a **pure** sort key from the nomen's numeric part

**`HGVSDisplay`** — keeps the name and becomes the small view wrapper it describes: `genome_build`, `is_normalised`, `is_desired_build`, `to_json()`, and the display-flavoured sort. `to_json()` emits exactly the `{transcript, gene_symbol, c_nomen, full, genome_build, desired, normalized}` shape that `VCTable.format_hgvs` in `vc_form.js` consumes, so it belongs at that boundary.

`annotation/templatetags/clinvar_tags.py` carries its own `is_desired_build` + `genome_build` on a separate `ClinVarDetails` dataclass — the same display concept re-invented, so the wrapper may be reusable beyond classification.

Attribute renames that travel with the core, since they hold `g.99087902A>C` today for g.HGVS-only records: `full_c_hgvs` → `full_hgvs`, `raw_c` / `c_dot` → `nomen`.

## While in the area

**Import cost.** `genes/hgvs/__init__.py` is four lines, two of them `from .hgvs import *` and `from .hgvs_matcher import *`. So `from genes.hgvs import HGVSDisplay` in a datatables view pulls in the whole 745-line matcher — cdot, `snpdb.clingen_allele`, the Django cache, both converter stacks. A leaf value object shouldn't cost that.

**Five hand-rolled HGVS regexes** — `HGVSDisplay.HGVS_REGEX`, `HGVSMatcher.HGVS_CLEAN_PATTERN_OPTIONAL_TRANSCRIPT`, and four in `imported_allele_info_view.py` (`HGVS_REGEX_BASIC` / `REF_ALT` / `DEL_INS` / `SIMPLE_OP`), alongside `cdot.hgvs.clean_hgvs`. `HGVSDisplay.HGVS_REGEX` is also reached into from outside the class (`classification_variant_info_models.py:901`).

**Naming.** `c_hgvs_best()`, `get_c_hgvs()`, `Classification.chgvs_grch37/38` and the `ResolvedVariantInfo.c_hgvs` column all hold a g.HGVS for g.HGVS-only records, so "c_hgvs means c.HGVS" is already untrue at the DB level. The column rename needs a migration, so it's worth deciding separately whether it's in scope.

## Suggested order

1. The pure deletions — `transcript_parts` delegate, `_clean_transcript`, `transcript_version_model`. Small, independently testable, and they shrink the class before anything else touches it.
2. Extract `HGVSComponents`, leaving `HGVSDisplay` as the wrapper. This is the step with real call-site churn, and where the two correctness edges above get fixed.
3. Attribute renames and the regex consolidation.

Context: the rename from `CHGVS` came out of #1063, where g.HGVS-only classifications exposed how much of this class assumes a transcript-based c.HGVS.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.