Add configurable ProForma residue validation

[ryan-fellers]

Summary

• Add a constructor-supplied Func<char, bool> validator to ProFormaParser.
• Keep the existing no-argument behavior by defaulting to char.IsUpper.
• Apply the validator to both sequence residues and global-modification targets, while reserving the target-list comma for ProForma grammar.
• Retarget the benchmark and test projects from .NET 6 to .NET 8 and update the build and release workflows to install .NET 8. The shipping library remains targeted at netstandard2.0;netstandard2.1, so consumer compatibility is unchanged.

This is a small extension point for applications that need to accept a locally defined residue alphabet, including non-standard amino-acid symbols, without changing default parser behavior. Workflows that create proteoform groups or chemical hashes must pair the parser with a matching custom IResidueProvider.

ProForma context

ProForma 2.0 identifies support for rare amino acids without an accepted one-letter code as a pending future-development item: ProForma 2.0 paper.

The ProForma 2.1 draft/schema explicitly includes unusual O/U and ambiguous B/J/X/Z, while enumerating the ASCII uppercase residue alphabet rather than defining generic notation for arbitrary non-natural residues: current schema. A related PSI discussion proposes an X-based known-mass representation for 2-aminoisobutyric acid and asks about PSI-MOD accession support: issue #27.

Follow-up

TopPicProformaParser still uses char.IsUpper for its residue-index map. Supporting custom alphabets in that separate conversion path is intentionally deferred from this parser-focused PR and tracked in #121.

Validation

• dotnet test tests\TopDownProteomics.Tests\TopDownProteomics.Tests.csproj --no-restore --filter FullyQualifiedName~ProFormaParserTests
• dotnet test tests\TopDownProteomics.Tests\TopDownProteomics.Tests.csproj --no-restore
• Both commands passed on net8.0 and net472; the full suite reports 254 passed and 7 existing skips per target.

[codecov]

Codecov Report

❌ Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.44%. Comparing base (e031543) to head (1ccfd68).
⚠️ Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
src/TopDownProteomics/ProForma/ProFormaParser.cs	90.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #120      +/-   ##
==========================================
+ Coverage   89.42%   89.44%   +0.02%     
==========================================
  Files         105      105              
  Lines        5851     5856       +5     
  Branches      912      913       +1     
==========================================
+ Hits         5232     5238       +6     
  Misses        430      430              
+ Partials      189      188       -1     

Files with missing lines	Coverage Δ
src/TopDownProteomics/ProForma/ProFormaParser.cs	92.89% <90.00%> (+0.09%)	⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[mikehollas123]

Spoke to Ryan,
Looks good to me

[trishorts]

Nice, well-scoped extension point — thanks for this. I read the whole parse loop rather than just the diff, and the placement is right: the residue check is the final else in ParseString, after every structural branch (< > [ ] ( ) { } -), so even a maximally permissive validator can't capture a ProForma grammar character out of the sequence. The default path really is unchanged.

Four small things inline, none of them blocking. One is a genuine (if narrow) correctness bug in the global-modification target loop, where the ordering is the reverse of the sequence loop's.

Two things that aren't inline:

The other half of this extension point already exists, and the PR doesn't mention it. The motivation you cite — rare amino acids with no accepted one-letter code — needs the residue lookup gate as well as the parser gate, and everything past ProFormaTerm goes through the former: ProteoformGroupFactory.CreateProteoformGroup does term.Sequence.Select(x => _residueProvider.GetResidue(x)) for every character (ProteoformGroupFactory.cs:51). Against the stock provider that means GetResidue('*') throws ArgumentException("Not an upper case amino acid symbol") (IupacAminoAcidProvider.cs:33), and 'X'/'B'/'J'/'Z' return an unpopulated null slot, since CreateResideArray fills 22 of 26 entries (O and U are there, at :162 and :169).

So new ProFormaParser(customValidator) parses a custom alphabet but can't yet be turned into a proteoform group. The good news is that the missing half is already public and pluggable: IResidueProvider has exactly one implementation, and ChemicalProteoformHashGenerator takes an injected ProFormaParser (ChemicalProteoformHashGenerator.cs:38), so a caller supplying both a custom validator and a custom IResidueProvider gets an end-to-end story today. That's a strong argument for the PR — it just isn't written down anywhere. A line in the constructor's <remarks> and in the PR description pointing at IResidueProvider would save the next reader from having to discover that seam.

Cosmetic: three blank lines are deleted inside Ambiguity_UnknownPosition_4_4_1 (old lines 784, 802, 826). The surrounding code is byte-identical and the method has nothing to do with residue validation — they were separating the three scenarios that test walks. Worth restoring to keep the diff to its stated payload.

Out of scope but noting it so it isn't lost: TopPicProformaParser.cs:119 still hard-codes char.IsUpper when building its residue index map, so TopPIC conversion would mis-index under a custom alphabet. Fine as a follow-up.

[trishorts]

Approving. Every point from my earlier review is addressed in 1ccfd680, and I've confirmed each one against the diff rather than the commit message:

• Parse error now reads "{current} is not a valid residue.", with the text pinned by an assertion.
• The global-modification target loop reserves , before consulting the validator, so the two loops now treat the grammar the same way. The added new ProFormaParser(_ => true) case against <[Oxidation]@A,C>A is exactly the scenario that previously produced {'A', ',', 'C'}.
• Assert.Throws<ArgumentNullException>(() => new ProFormaParser(null!)) covers the one previously-unexercised branch.
• The three unrelated blank-line deletions are restored.
• Both workflows are on 8.x, and the PR description now declares the retarget and confirms the shipping library stays on netstandard2.0;netstandard2.1.
• The constructor <remarks> now points at IResidueProvider, which is what makes the feature usable end-to-end.

I also checked that the comma fix is inert for existing callers: char.IsUpper(',') was already false and the old else if (tagText[k] != ',') already skipped it, so no current consumer sees a behavior change.

One caveat on the record, since it isn't something the diff can tell you. CI has not run on this head. Because the PR now edits .github/workflows/, GitHub gated the run — Build and Test on 1ccfd680 returned action_required after 0s, and there are currently no check-runs or statuses on the commit. The green build and red codecov/patch still displayed on this PR are stale results from 09dad6c.

So this approval is a code review, not a green pipeline. Someone with maintainer rights should click Approve and run and confirm build passes and codecov/patch clears before merging — particularly since the workflow change is itself the thing that needs proving.