Skip to content

Conversation

@jathavaan
Copy link
Collaborator

This pull request refactors the way SQL filter strings are constructed for FKB and OSM IDs in the merge_fkb_osm method to improve readability and maintainability. The logic for building the filter strings is now separated into explicit string variables before use.

Refactoring for SQL filter construction:

  • Refactored construction of the fkb_filter and osm_filter SQL filter strings by first joining the ID lists into string variables (ids_fkb_str and ids_osm_str), improving clarity and reducing code duplication. (src/infra/infrastructure/services/conflation_service.py, src/infra/infrastructure/services/conflation_service.pyL181-R185)

@jathavaan jathavaan self-assigned this Nov 17, 2025
Copilot AI review requested due to automatic review settings November 17, 2025 21:03
@jathavaan jathavaan merged commit 5f129d2 into main Nov 17, 2025
@jathavaan jathavaan deleted the bugfix/39-fix-aci-error-with-backslash-in-string branch November 17, 2025 21:03
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 pull request fixes a bug with an unterminated f-string in SQL filter construction and refactors the code for improved readability by separating the string joining logic from the conditional filter creation.

  • Extracts ID list joining into separate variables (ids_fkb_str, ids_osm_str)
  • Simplifies the ternary operator expressions for fkb_filter and osm_filter
  • Resolves the unterminated f-string syntax issue mentioned in the PR title

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

Comment on lines +181 to +182
ids_fkb_str = ", ".join(f"'{v}'" for v in ids_fkb)
ids_osm_str = ", ".join(f"'{v}'" for v in ids_osm)
Copy link

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

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

[nitpick] The string joining operations are performed unconditionally, even when ids_fkb or ids_osm are empty lists. This creates unnecessary string operations that won't be used when the filters evaluate to "FALSE". Consider moving the join operations inside the ternary expressions or using lazy evaluation to avoid this overhead.

Example:

fkb_filter = "FALSE" if not ids_fkb else f"external_id IN ({', '.join(f\"'{v}'\" for v in ids_fkb)})"
osm_filter = "FALSE" if not ids_osm else f"external_id IN ({', '.join(f\"'{v}'\" for v in ids_osm)})"

Copilot uses AI. Check for mistakes.
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.

1 participant