Skip to content

Feature/st id mapping rework#48

Merged
jweiser merged 4 commits intomainfrom
feature/st-id-mapping-rework
Feb 2, 2026
Merged

Feature/st id mapping rework#48
jweiser merged 4 commits intomainfrom
feature/st-id-mapping-rework

Conversation

@jweiser
Copy link
Copy Markdown
Member

@jweiser jweiser commented Jan 29, 2026

Adds StableIdMapping data structure and filter out mis-matches between species in stable id mappings

@jweiser jweiser self-assigned this Jan 29, 2026
@jweiser jweiser added bug Something isn't working enhancement New feature or request labels Jan 29, 2026
Copy link
Copy Markdown
Contributor

@adamjohnwright adamjohnwright left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some suggestions from claude:

  1. Package naming convention - The new package is StableIdMapper (PascalCase), but Java convention is lowercase package names (e.g., stableidmapper or stableid). Consider renaming for consistency.
  2. Null safety in StableIdMapping - What happens if an empty list is passed to the constructor? Does getPrimaryId() throw an IndexOutOfBoundsException? Consider adding validation or returning Optional.
  3. Test coverage - Are there unit tests for the new StableIdMapping class? The filtering logic in filterOutMismatchedIdentifiers() and species abbreviation extraction would benefit from explicit test cases, especially edge cases (single ID, all mismatched, etc.). 4. Immutability - Consider making StableIdMapping immutable by storing a defensive copy of the input list and making fields final. This prevents subtle bugs if the original list is modified elsewhere.
  4. Species abbreviation extraction - The logic extracting the species code (e.g., "HSA" from "R-HSA-12345") should be documented or extracted into a named method for clarity. What happens with malformed IDs that don't follow the expected pattern?
  5. The double-negative was confusing - The original condition !(stableIds.size() < 2) (meaning "size >= 2") was hard to read. Good cleanup, but ensure the new logic is semantically identical.

you can choose to do them or not.

@jweiser jweiser merged commit 4a853bb into main Feb 2, 2026
1 check passed
@jweiser jweiser deleted the feature/st-id-mapping-rework branch February 2, 2026 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants