Skip to content

Conversation

@jweiser
Copy link
Member

@jweiser jweiser commented Aug 23, 2023

Checks "deletedStableIdentifier" for referrers of stable identifier instances in "_DeletedInstance" (in addition to the normal usage of stable identifiers).

@jweiser jweiser added bug Something isn't working enhancement New feature or request labels Aug 23, 2023
@jweiser jweiser self-assigned this Aug 23, 2023
@jweiser jweiser changed the base branch from main-curator to main January 29, 2026 19:45
Copy link

@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.

looks good to me. here are suggestions from Claude:

Questions / Suggestions

  1. 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.
  2. 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.

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.

3 participants