adds deleted stable identifiers for checks if stable id is used - #60
Conversation
adamjohnwright
left a comment
There was a problem hiding this comment.
looks good to me. here are suggestions from Claude:
Questions / Suggestions
- Magic string - "deletedStableIdentifier" is used as a raw string while ReactomeJavaConstants.stableIdentifier uses a constant. Is there a corresponding constant like ReactomeJavaConstants.deletedStableIdentifier? If so, prefer that for consistency.
If not, consider adding one or leaving a comment explaining why it's hardcoded. - Wildcard imports - Changing to import java.util.* is generally discouraged in many codebases as it:
- Reduces clarity about which classes are actually used
- Can cause naming conflicts
- Makes it harder to track dependencies
Consider keeping explicit imports unless the project style guide allows wildcards.
3. Empty vs null check - The condition changed from referrers == null || referrers.size() == 0 to just referrers.size() == 0. This works because referrers is now initialized as new ArrayList<>(), but it's a subtle change worth noting. Using
referrers.isEmpty() would be more idiomatic.
4. Formatting churn - The PR shows +66/-60 but the functional change is small. The bulk appears to be indentation/formatting changes (spaces to tabs). This makes the diff harder to review. Consider separating formatting changes from logic changes in
future PRs, or at minimum noting this in the PR description.
5. Test coverage - Is there a test case that verifies stable IDs referenced only by deleted instances are no longer flagged? This would prevent regression.
6. Comment update - Was the comment at line 51 updated to reflect the new behavior? It should indicate that both active and deleted instance references are now considered.
Checks "deletedStableIdentifier" for referrers of stable identifier instances in "_DeletedInstance" (in addition to the normal usage of stable identifiers).