Skip to content

Conversation

@mmathieum
Copy link
Member

@mmathieum mmathieum self-assigned this Nov 28, 2025
@mmathieum
Copy link
Member Author

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a valuable feature for verifying the uniqueness of cleaned IDs for services, routes, trips, and stops. The implementation is mostly correct and follows the existing patterns in the codebase.

My main concerns are:

  • A potential NullPointerException in DefaultAgencyTools due to missing null checks, which could cause runtime failures.
  • The new verification logic in MVerify.kt is highly repetitive and could be significantly simplified and improved for better error reporting.

I've provided specific suggestions to address these points. Additionally, the pattern of adding maps and methods for each ID type in DefaultAgencyTools is also quite repetitive, which is a minor maintainability concern you might want to address in the future.

@mmathieum mmathieum requested a review from Copilot November 28, 2025 16:05
@mmathieum mmathieum marked this pull request as ready for review November 28, 2025 16:05
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds optional verification to ensure that cleaned IDs (service, route, trip, and stop IDs) remain unique after cleanup transformations are applied. The verification is opt-in and only runs when cleanup regex patterns are configured, with an additional option to explicitly allow non-unique IDs.

Key Changes:

  • Introduced a new MVerify class that checks for duplicate cleaned IDs and throws an error if duplicates are found
  • Added tracking maps to store original-to-cleaned ID mappings for all four ID types
  • Added configuration options to explicitly allow non-unique IDs when needed

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
MVerify.kt New verification utility that checks cleaned ID uniqueness across service, route, trip, and stop IDs
GAgencyTools.java Added interface methods for uniqueness verification flags
RouteConfig.kt Added configuration flags to allow non-unique route, trip, and stop IDs
AgencyConfig.kt Added configuration flag to allow non-unique service IDs
DefaultAgencyTools.java Implemented ID tracking maps, verification methods, and integrated verification into the generation process

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@mmathieum mmathieum merged commit 0386386 into master Nov 28, 2025
4 checks passed
@mmathieum mmathieum deleted the mm/cleanup_ids_verified branch November 28, 2025 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Option to verify uniqueness of cleaned IDs (route, stop, trip, service)

2 participants