Read on/off method-file keys in both directions - #813
Merged
Merged
Conversation
Several ConfigParser arms assigned only one of "true"/"false" and returned true for either, so the other value was recorded as applied and ignored. "Keep original precursor isotopes: True" never took effect, because the property defaults to false. Route every boolean arm through a Flag helper that accepts both values case-insensitively and reports anything else as an unusable value. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-boolean-keys # Conflicts: # tests/MSDIAL5/MsdialCoreTestApp/Parser/ConfigParser.cs
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-boolean-keys # Conflicts: # tests/MSDIAL5/MsdialCoreTestApp/Parser/ConfigParser.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Several
casearms in the MS-DIAL 5 console'sConfigParserassigned only one of the two boolean values, but returnedtrueeither way:KeepOriginalPrecursorIsotopesdefaults tofalse, soKeep original precursor isotopes: Truechanged nothing, yet the method-file key report (MethodFileKeys) listed it as applied. The other one-way arms were safe only because their default happened to match the one value they accepted.Change
TrueOrFalse(string text, Action<bool> assign)helper next toNumberandCount. It acceptstrueandfalsein any letter case. Any other value returnsMethodKeyOutcome.UnusableValue, so the key report says the value could not be read.if (valueLower == "true" || valueLower == "false") ... = bool.Parse(...); return true;) now use it too, because they also reported an unreadable value as applied. Each was a one-line change.TrueOrFalsehelper that does the same job for its line readers. This PR uses that name for the shared helper, so there is one helper, not two. The three line-reader callers (ReadAlignmentLightMode,ReadDetailedAlignmentProvenance,ReadAnnotationCandidateExport) behave as before.ReadAlignmentLightMode,ReadDetailedAlignmentProvenanceandReadAnnotationCandidateExport. An unreadable value still falls back tofalsethere, as before.Keys whose behaviour changes
Now read in both directions. A value that used to be silently ignored now takes effect, and any value other than true/false is reported as unusable:
Keep original precursor isotopesfalse(Truewas ignored, and it is the non-default value)Exclude after precursorfalseCorrDec executefalseIs private versiontrueIs private version of TADAtrueAccumulate MS2 spectratrueReplace quant mass by user defined valuetrueIs quant mass based on base peak mztrueAlready two-way, but an unreadable value (e.g.
yes) is now reported as unusable instead of applied:TrueOrFalsewhen merging master)A method file that spelled one of these as anything other than true/false was already being ignored. The run itself is unchanged; the key report now says so.
LBM CCS filtering key
use ccs for lbm-based annotation filteringused to set the MSP parameter instead of the LBM one. #816 fixed that on master, and this PR keeps #816's fix after merging it.Tests
Trueapplies,Falseapplies,TRUEapplies, andyesis reported as unusable and leaves the previous value alone.Together with alignment) is covered with the same checks.dotnet test tests/MSDIAL5/MsdialCoreTestAppTests/MsdialCoreTestAppTests.csproj: 63 passed, 0 failed (after merging master).dotnet build tests/MSDIAL5/MsdialCoreTestApp/MsdialCoreTestApp.csprojsucceeds for net472, net48 and net8.🤖 Generated with Claude Code