Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the MSP file parser by updating the query collections to use a HashSet for deduplication and refining the query checking logic.
- Replace list-based queries with a HashSet in the LbmFileReader and ReadSerializedLbmLibrary methods
- Change the queryCheck method parameter type to IReadOnlyCollection and update its switch-case logic conditions
Comments suppressed due to low confidence (2)
src/Common/CommonStandard/Parser/MspFileParcer.cs:221
- The parameter name 'collosionType' appears to be misspelled; consider renaming it to 'collisionType' for clarity.
private static bool queryCheck(MoleculeMsReference mspRecord, IReadOnlyCollection<string> queries, IonMode ionMode, SolventType solventType, CollisionType collosionType)
src/Common/CommonStandard/Parser/MspFileParcer.cs:230
- The logic in the switch-case for IonMode.Negative has been inverted compared to the original expression; please verify that the new conditions correctly capture the intended behavior.
case SolventType.CH3COONH4 when mspRecord.AdductType.IsFA || mspRecord.AdductType.IsHco3:
There was a problem hiding this comment.
Pull Request Overview
This PR aims to fix the MSP file parser functionality by updating the queryCheck logic and improving the efficiency of query handling in the MSP file reader functions.
- Introduces new test methods in MspFileParserTests.cs to validate adduct type filtering per solvent type.
- Updates the MspFileParcer.cs to use a HashSet for query strings and revises the switch condition in queryCheck to correct logical errors.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/Common/CommonStandardTests/Parser/MspFileParserTests.cs | Added tests for validating queryCheck behavior with different adduct types. |
| src/Common/CommonStandard/Parser/MspFileParcer.cs | Modified queryCheck to improve performance (using HashSet) and corrected solvent type condition logic. |
Comments suppressed due to low confidence (1)
src/Common/CommonStandard/Parser/MspFileParcer.cs:1
- [nitpick] The file name 'MspFileParcer.cs' appears to be a misspelling of 'MspFileParser.cs'; if unintentional, please consider renaming to improve clarity.
File name is 'MspFileParcer.cs'
| { | ||
| case SolventType.CH3COONH4 when !mspRecord.AdductType.IsHac: | ||
| case SolventType.HCOONH4 when !mspRecord.AdductType.IsFA: | ||
| case SolventType.NH4HCO3 when !mspRecord.AdductType.IsHco3: | ||
| case SolventType.CH3COONH4 when mspRecord.AdductType.IsFA || mspRecord.AdductType.IsHco3: | ||
| case SolventType.HCOONH4 when mspRecord.AdductType.IsHac || mspRecord.AdductType.IsHco3: |
There was a problem hiding this comment.
[nitpick] Consider adding inline comments to clarify the intended behavior behind the updated solvent type conditions in the queryCheck method, ensuring future maintainers can follow the logic changes.
No description provided.