Skip to content

Minor correctness and clarity cleanups#178

Merged
msallin merged 1 commit into
masterfrom
chore/minor-correctness-cleanups
Jun 16, 2026
Merged

Minor correctness and clarity cleanups#178
msallin merged 1 commit into
masterfrom
chore/minor-correctness-cleanups

Conversation

@msallin

@msallin msallin commented Jun 16, 2026

Copy link
Copy Markdown
Owner

Summary

Three small, behavior-preserving cleanups surfaced during the correctness review.

Changes

  • AssociationTypeContainer: materialize the Select projection with ToList(). GetAssociationTypes is called once per entity set; the deferred query previously rebuilt every SqliteAssociationType (and its entity-set lookups) on each call.
  • ConnectionStringParser.GetDataSource: reuse the dictionary already returned by ParseConnectionString instead of parsing the connection string a second time.
  • SqliteForeignKeyIndexConvention: when disambiguating duplicate indices on the same property, fold the _{count} suffix into the property name before escaping. IndexNameCreator.CreateName returns an already-quoted identifier, so the old code appended the suffix after the closing quote ("IX_T_P"_1); it now stays inside the quotes ("IX_T_P_1"). The normal (count == 0) path is unchanged.

Tests

No new tests: the first two are pure refactors covered by the existing suite, and the third only changes the "should never happen" duplicate-index path while leaving normal output identical (verified by the unchanged SQL-generation reference tests). Full suite green (47 tests).

@coderabbitai

coderabbitai Bot commented Jun 16, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: f79dd158-df03-42b9-a0c7-2a057298f55d

📥 Commits

Reviewing files that changed from the base of the PR and between 6580b11 and 6718e1e.

📒 Files selected for processing (3)
  • SQLite.CodeFirst/Internal/Convention/SqliteForeignKeyIndexConvention.cs
  • SQLite.CodeFirst/Internal/Utility/AssociationTypeContainer.cs
  • SQLite.CodeFirst/Internal/Utility/ConnectionStringParser.cs
✅ Files skipped from review due to trivial changes (1)
  • SQLite.CodeFirst/Internal/Utility/ConnectionStringParser.cs
🚧 Files skipped from review as they are similar to previous changes (2)
  • SQLite.CodeFirst/Internal/Utility/AssociationTypeContainer.cs
  • SQLite.CodeFirst/Internal/Convention/SqliteForeignKeyIndexConvention.cs

📝 Walkthrough

Walkthrough

Three small internal fixes: SqliteForeignKeyIndexConvention.CreateIndexAnnotation now folds the duplicate-count suffix into the property name before calling IndexNameCreator.CreateName; AssociationTypeContainer eagerly materializes its association-type list via ToList() at construction; ConnectionStringParser.GetDataSource reuses the already-parsed dictionary instead of re-parsing the connection string.

Changes

Internal utility and convention fixes

Layer / File(s) Summary
Index naming order, eager materialization, and connection-string re-parse removal
SQLite.CodeFirst/Internal/Convention/SqliteForeignKeyIndexConvention.cs, SQLite.CodeFirst/Internal/Utility/AssociationTypeContainer.cs, SQLite.CodeFirst/Internal/Utility/ConnectionStringParser.cs
CreateIndexAnnotation merges the count suffix into uniquePropertyName before passing to IndexNameCreator.CreateName; AssociationTypeContainer calls ToList() to prevent deferred re-evaluation; GetDataSource replaces a redundant ParseConnectionString call with the already-computed strings[DataSourceToken].

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Poem

🐇 Hop, hop, a tidy little fix,
No more parsing the same string twice!
Count the suffix early, that's the trick,
And eager lists keep queries nice.
Three small leaps make the codebase bright —
This rabbit's burrow feels just right! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Minor correctness and clarity cleanups' accurately summarizes the PR's main focus on three behavior-preserving refactoring improvements.
Description check ✅ Passed The description clearly explains all three changes, their motivations, testing approach, and confirms no test suite regressions, directly supporting the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore/minor-correctness-cleanups

Comment @coderabbitai help to get the list of available commands and usage tips.

- AssociationTypeContainer: materialize the projection with ToList so
  GetAssociationTypes does not rebuild every SqliteAssociationType on each call.
- ConnectionStringParser.GetDataSource: reuse the already-parsed dictionary
  instead of parsing the connection string a second time.
- SqliteForeignKeyIndexConvention: fold the uniqueness suffix into the index
  name before escaping so it stays inside the quoted identifier instead of being
  appended after the closing quote.
@msallin msallin force-pushed the chore/minor-correctness-cleanups branch from 6580b11 to 6718e1e Compare June 16, 2026 04:25
@sonarqubecloud

Copy link
Copy Markdown

@msallin msallin merged commit b1cafcc into master Jun 16, 2026
4 checks passed
@msallin msallin deleted the chore/minor-correctness-cleanups branch June 16, 2026 04:28
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