Skip to content

Commit 2d6cd40

Browse files
holmrenserclaude
andcommitted
Fix review findings: correctness, robustness, infra
Correctness (P0): - Encode protein sequences case-insensitively (lowercase no longer maps to X) - Single-quote Newick labels containing whitespace/reserved characters Robustness (P1): - Add NJError::DuplicateIdentifier; reject duplicate names in validate_msa - Replace bitset_of panic with a propagated error - Add fallible MSA::try_from_pairs; use it instead of panicking from_iter - Warn (NJEvent::Log) on all-gap sequences; document degenerate-input contracts Maintainability & error fidelity (P2): - Collapse the model x alphabet dispatch into a single macro - Share one bootstrap replicate fn; propagate failures instead of .expect panics - Python: typed exceptions (ValueError/RuntimeError) + surfaced callback errors - WASM: throw Error with stable .name code; log callback errors to console Quality infrastructure (P3): - criterion benchmarks (make bench) - proptest property tests + golden-tree tests Release/CI/packaging (P4): - scripts/check-versions.sh + CI job; release.sh uses a fresh commit (no --amend) - abi3-py310 single-wheel build; pin Node 22 in release to match CI - package.json exports map; add CHANGELOG.md and CONTRIBUTING.md All suites green: nj (default/cli/parallel) + golden/properties, WASM web+node, Python (abi3 wheel). No new clippy warnings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 29bb9fd commit 2d6cd40

29 files changed

Lines changed: 1617 additions & 222 deletions

.github/workflows/release.yml

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@ jobs:
1616
runs-on: ubuntu-latest
1717
steps:
1818
- uses: actions/checkout@v4
19+
- name: All package manifests agree
20+
run: bash scripts/check-versions.sh
1921
- name: Check tag matches workspace version
2022
run: |
2123
TAG="${GITHUB_REF_NAME#v}"
@@ -57,19 +59,16 @@ jobs:
5759
- run: cp README.md python/README.md
5860
- uses: actions/setup-python@v5
5961
with:
60-
# One Python is enough to *find* an interpreter, but for wheels you
61-
# generally want to build multiple; see args below.
6262
python-version: "3.12"
6363
- uses: PyO3/maturin-action@v1
6464
with:
6565
command: build
66-
# Explicitly provide interpreters so maturin doesn't have to discover them.
66+
# The crate is built with pyo3's `abi3-py310` feature, so a single
67+
# stable-ABI wheel per platform covers CPython 3.10+ — no per-minor
68+
# interpreter list needed.
6769
args: >
6870
--release
6971
--out dist
70-
-i python3.10
71-
-i python3.11
72-
-i python3.12
7372
target: ${{ matrix.target }}
7473
manylinux: ${{ matrix.manylinux }}
7574
working-directory: python
@@ -157,7 +156,7 @@ jobs:
157156
workspaces: wasm -> wasm/target
158157
- uses: actions/setup-node@v4
159158
with:
160-
node-version: "24"
159+
node-version: "22"
161160
registry-url: https://registry.npmjs.org
162161
- run: cp README.md wasm/README.md
163162
- run: npm ci

.github/workflows/test.yml

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,13 @@ on:
88
workflow_call:
99

1010
jobs:
11+
versions:
12+
name: Version consistency
13+
runs-on: ubuntu-latest
14+
steps:
15+
- uses: actions/checkout@v4
16+
- run: bash scripts/check-versions.sh
17+
1118
rust:
1219
name: Rust tests (${{ matrix.rust }})
1320
runs-on: ubuntu-latest

CHANGELOG.md

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
# Changelog
2+
3+
All notable changes to this project are documented here. The format is loosely
4+
based on [Keep a Changelog](https://keepachangelog.com/), and the project follows
5+
[Semantic Versioning](https://semver.org/). The three published artifacts —
6+
the `nj` crate (crates.io), `nj_py` (PyPI), and `@holmrenser/nj` (npm) — share a
7+
single version, bumped together via `make bump-{patch,minor,major}`.
8+
9+
## [Unreleased]
10+
11+
### Fixed
12+
- Protein sequences are now encoded case-insensitively; lowercase residues were
13+
previously mapped to `X`, silently corrupting protein alignments.
14+
- Newick output now single-quotes identifiers containing whitespace or reserved
15+
characters (`()[]{}',:;`), producing valid Newick for names with spaces, etc.
16+
- Bootstrap replicate failures now propagate as errors instead of panicking a
17+
worker thread.
18+
19+
### Added
20+
- Duplicate sequence identifiers are rejected with `NJError::DuplicateIdentifier`
21+
(they previously corrupted bootstrap clade counting).
22+
- A `Warning` log event is emitted for all-gap sequences (whose distances are 0).
23+
- Language bindings surface error kind: Python raises `ValueError` for usage
24+
errors and `RuntimeError` for internal failures; WASM throws an `Error` whose
25+
`name` is the stable error code. Failing user callbacks are logged rather than
26+
silently dropped.
27+
- Criterion benchmarks (`make bench`) and proptest property + golden-tree tests.
28+
- `scripts/check-versions.sh` and a CI job that verify the three package
29+
versions stay in sync.
30+
31+
### Changed
32+
- Python wheels are built once per platform as a stable-ABI (`abi3-py310`) wheel
33+
covering CPython 3.10+, replacing the per-minor-version build matrix.
34+
35+
## [0.0.22] and earlier
36+
37+
See the Git tag history (`git tag`) and the GitHub releases page for prior
38+
versions.

CONTRIBUTING.md

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
# Contributing
2+
3+
Thanks for your interest in contributing to `nj.rs`! This is a Cargo workspace
4+
that ships three artifacts from one core crate:
5+
6+
- `nj/` — core Rust library + CLI (the `cli` and `parallel` features are optional)
7+
- `python/` — PyO3 bindings published to PyPI as `nj_py`
8+
- `wasm/` — wasm-bindgen bindings published to npm as `@holmrenser/nj`
9+
10+
## Building & testing
11+
12+
```bash
13+
make all # build library, CLI, Python, and WASM
14+
make test # run Rust (incl. cli + parallel), WASM (node + web), and Python tests
15+
make bench # run the criterion benchmarks
16+
```
17+
18+
You can also test pieces individually:
19+
20+
```bash
21+
cargo test -p nj # core unit + integration tests
22+
cargo test -p nj --features parallel # exercise the parallel bootstrap path
23+
make -C python test # maturin develop + pytest (needs maturin + uv)
24+
make -C wasm test # wasm-pack build + node/web tests (needs wasm-pack + node)
25+
```
26+
27+
Please add tests with any behavioural change. Cross-cutting changes to the core
28+
API should be reflected in the Python (`python/tests/`) and WASM
29+
(`wasm/tests/`) suites where relevant.
30+
31+
## Documentation single source of truth
32+
33+
The root `README.md` is the **only** committed README. It is copied into
34+
`python/` and `wasm/` at release time and is git-ignored there, so never commit
35+
`python/README.md` or `wasm/README.md`.
36+
37+
## Releasing
38+
39+
All three packages share one version. Bump them together — never edit the
40+
version strings by hand:
41+
42+
```bash
43+
make bump-patch # or bump-minor / bump-major
44+
```
45+
46+
`release.sh` (invoked by `make bump-*`) requires `cargo-workspaces`, `toml`,
47+
`jq`, and `sponge` on `PATH`. It bumps the Cargo workspace version, syncs
48+
`python/pyproject.toml` and `wasm/package.json`, runs `scripts/check-versions.sh`
49+
to confirm they agree, then creates a `Release vX.Y.Z` commit and tag.
50+
51+
Pushing a `vX.Y.Z` tag triggers `.github/workflows/release.yml`, which runs the
52+
full test matrix, re-verifies the version, and publishes to crates.io, PyPI
53+
(OIDC trusted publishing), and npm (OIDC + provenance).
54+
55+
You can sanity-check version consistency at any time:
56+
57+
```bash
58+
bash scripts/check-versions.sh
59+
```

0 commit comments

Comments
 (0)