Skip to content

fix: v0.5.3 — PR review fixes (orderBy consistency, redundant casts) - #40

Merged
teetangh merged 1 commit into
mainfrom
fix/v0.5.3-review-fixes
Mar 28, 2026
Merged

fix: v0.5.3 — PR review fixes (orderBy consistency, redundant casts)#40
teetangh merged 1 commit into
mainfrom
fix/v0.5.3-review-fixes

Conversation

@teetangh

Copy link
Copy Markdown
Owner

Summary

Addresses 3 review comments from PR #39:

  1. HIGH: findFirstRaw now supports List orderBy for multi-column sorting (was Map-only, inconsistent with findManyRaw)
  2. MEDIUM: Removed redundant as Map<String, dynamic> casts in findMany orderBy (Dart type promotion handles it)
  3. MEDIUM: Removed redundant as Map<String, dynamic> cast in findManyRaw orderBy

Test plan

  • All unit tests pass
  • flutter analyze --fatal-infos — zero issues
  • dart format --set-exit-if-changed . — zero changes

🤖 Generated with Claude Code

- Remove redundant `as` casts after `is` checks (Dart type promotion)
- Add List orderBy support to findFirstRaw (was Map-only, now matches findManyRaw)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

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

Copy link
Copy Markdown

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 updates the Prisma Flutter Connector to version 0.5.3, removing redundant type casts and adding support for List-based orderBy in findFirst queries. The reviewer suggests refactoring the orderBy type-checking logic into if-else if chains to improve code clarity and performance by explicitly handling the mutually exclusive nature of the types.

Comment on lines +204 to +206
if (orderBy is Map<String, dynamic>) queryBuilder.orderBy(orderBy);
if (orderBy is List) queryBuilder.orderBy(orderBy);
if (orderBy is ${m}OrderByInput) queryBuilder.orderBy(_orderByToJson(orderBy as ${m}OrderByInput));
if (orderBy is ${m}OrderByInput) queryBuilder.orderBy(_orderByToJson(orderBy));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

For clarity and minor performance improvement, it's better to use an if-else if chain here. Since orderBy can only be one of these types at a time, this structure more clearly communicates the exclusive nature of these checks.

      if (orderBy is Map<String, dynamic>) queryBuilder.orderBy(orderBy);
      else if (orderBy is List) queryBuilder.orderBy(orderBy);
      else if (orderBy is ${m}OrderByInput) queryBuilder.orderBy(_orderByToJson(orderBy));

Comment on lines +272 to 273
if (orderBy is Map<String, dynamic>) queryBuilder.orderBy(orderBy);
if (orderBy is List) queryBuilder.orderBy(orderBy);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Similar to my other comment, using an if-else if chain would be slightly more efficient and clearer here, as orderBy can't be both a Map and a List.

      if (orderBy is Map<String, dynamic>) queryBuilder.orderBy(orderBy);
      else if (orderBy is List) queryBuilder.orderBy(orderBy);

Comment on lines +310 to +311
if (orderBy is Map<String, dynamic>) queryBuilder.orderBy(orderBy);
if (orderBy is List) queryBuilder.orderBy(orderBy);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

To maintain consistency and improve clarity, an if-else if structure is preferable here as well. It makes it explicit that only one of the conditions will be executed.

      if (orderBy is Map<String, dynamic>) queryBuilder.orderBy(orderBy);
      else if (orderBy is List) queryBuilder.orderBy(orderBy);

@teetangh
teetangh merged commit d9745d2 into main Mar 28, 2026
8 of 12 checks passed
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