feat(llc): Update API specs - #118
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #118 +/- ##
==========================================
+ Coverage 85.53% 86.20% +0.67%
==========================================
Files 124 124
Lines 4342 4292 -50
==========================================
- Hits 3714 3700 -14
+ Misses 628 592 -36 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
f302c42 to
b53ee4a
Compare
xsahil03x
left a comment
xsahil03x
left a comment
There was a problem hiding this comment.
Reviewed the non-generated changes plus scripts/generate.sh, spot-checking the generated output to verify the hand-written adaptations. Comments cluster on three things: the unknown constants quietly changed meaning, the domain-type story is left half applied, and a few sharp edges in the script.
The script rewrite is a clear improvement overall — the spec version stamp in api.dart and the self-disabling sumScores fix are both much better than what they replace.
Note before any of the below matters: the branch is stale. stream_core needs main's f83b5d4d pin (3 commits ahead of the 813026f2 here — stream_datetime_converter.dart is in there, and 265 generated files reference StreamDateTimeConverter, so the package doesn't compile as pinned). That plus a rebase clears all nine analyzer errors.
| /// | ||
| /// Both sides are `String`-backed, so any value the API sends is carried over | ||
| /// verbatim — including ones this client version doesn't know about yet. | ||
| ActivityRestrictReplies toModel() => ActivityRestrictReplies(this); |
There was a problem hiding this comment.
The pass-through is right, but it means the domain unknown constants stopped being sentinels. ActivityRestrictReplies.unknown used to mean "a value this client didn't recognize"; now unrecognized values come through verbatim and the constant only matches the literal string "unknown", which the server never sends.
Same for ActivityDataVisibility.unknown (activity_data.dart:309), CollectionStatus.unknown (collection_data.dart:69), FeedVisibility.unknown (feed_input_data.dart:85), FeedMemberStatus.unknown (feed_member_data.dart:80) and FollowStatus.unknown (follow_data.dart:130) — nothing produces any of the six anymore.
Two asks: drop the constants, and add a CHANGELOG BREAKING line. Existing code like if (activity.restrictReplies == ActivityRestrictReplies.unknown) used to catch unrecognized values and now silently never fires. The current entry only covers the generated enums losing unknown.
| /// | ||
| /// Both sides are `String`-backed, so any value the API sends is carried over | ||
| /// verbatim — including ones this client version doesn't know about yet. | ||
| ActivityDataVisibility toModel() => ActivityDataVisibility(this); |
There was a problem hiding this comment.
Separate thought, since this is the second identical mapper: the migration leaves the domain-type story half applied.
Keep the domain types. The generated types are fragmented per message shape — five for feed visibility (FeedInputVisibility, FeedRequestVisibility, FeedResponseVisibility, FeedSuggestionResponseVisibility, ChangeFeedVisibilityRequestVisibility), four for activity visibility, four for restrictReplies, four for pushPreference, two for comment status. Collapsing those into one public name with one set of constants is what ActivityDataVisibility / FeedVisibility are for, and it keeps the public API off the generator's churn — this same PR renamed BanResponse to ModerationBanResponse. FeedOwnCapability is the exception that proves the rule: the spec consolidated it into a single shared schema, so the wrapper had nothing left to collapse and dropping FeedResponseOwnCapabilitiesMapper was right.
But the enum mappers can go. toModel() => ActivityDataVisibility(this) is pure type conversion, and the constructor says that more clearly at the call site because it names the target:
visibility: ActivityDataVisibility(visibility),They also don't scale — being extensions on the generated type, each of the four …Visibility types needs its own copy, while the constructor takes any of them. Six exported names and ~60 lines. (toRequest() stays; that one still does work.)
Three fields drift the other way, taking the raw generated value into a bare String and discarding the domain type: FeedData.visibility is String? while FeedInputData.visibility is FeedVisibility? (same SDK, typed on the way in, untyped on the way out); CommentData.status is String against two generated types, so users end up hardcoding 'active'/'deleted'; FollowData.pushPreference is String against four. None are regressions — all three were String before — but this is the PR where the convention gets set, so worth finishing or dropping deliberately.
| @@ -4748,7 +4748,7 @@ void main() { | |||
| createDefaultActivityResponse( | |||
| id: 'activity-1', | |||
| feeds: [feedId.rawValue], | |||
| restrictReplies: ActivityResponseRestrictReplies.unknown, | |||
| restrictReplies: ActivityResponseRestrictReplies.fromJson('unknown'), | |||
There was a problem hiding this comment.
This test is now tautological — it feeds the literal 'unknown' and asserts it equals ActivityRestrictReplies.unknown, which is also 'unknown'. Forward compatibility is the headline benefit of the extension-type migration and it currently has no coverage. Feeding a genuinely unrecognized value would test it:
restrictReplies: ActivityResponseRestrictReplies.fromJson('some_future_value'),
// ...
expect(
activity.restrictReplies,
equals(const ActivityRestrictReplies('some_future_value')),
);Test name wants updating to match ("unrecognized" rather than "unknown").
| @@ -158,39 +158,19 @@ extension FeedResponseMapper on FeedResponse { | |||
| ), | |||
| memberCount: memberCount, | |||
| name: name, | |||
| ownCapabilities: ownCapabilities?.map((e) => e.toModel()).toList() ?? const [], | |||
| ownCapabilities: [...?ownCapabilities], | |||
There was a problem hiding this comment.
[...?ownCapabilities] allocates a growable copy on every mapping. Now that the generated field is already List<FeedOwnCapability>, the previous ownCapabilities ?? const [] is equivalent and cheaper. Same at feed_suggestion_data.dart:78.
| ' "$SPEC_PATH" | ||
| # A partially applied rename produces a spec that references a schema that | ||
| # no longer exists, so fail here rather than deep inside the generator. | ||
| if grep -q "\#/components/schemas/${old}\b" "$SPEC_PATH"; then |
There was a problem hiding this comment.
Good guard. The other half isn't covered: if $new already exists as a schema, the rename yields two DurationResponse: keys and the generator takes whichever the YAML parser resolves last. Cheap to check before applying the rename:
if grep -q "^ ${new}:\$" "$SPEC_PATH"; then
echo "❌ Rename ${old} -> ${new} collides with an existing schema"; exit 1
fi| exit 1 | ||
| fi | ||
| echo "• Renamed ${old} -> ${new}" | ||
| done < <(perl -0777 -ne 'while (/"([^"]+)"\s*:\s*"([^"]+)"/g) { print "$1\t$2\n" }' "$RENAMED_MODELS") |
There was a problem hiding this comment.
This regex-scans the raw JSON for string pairs, so any nesting or extra key silently becomes a rename. Fine for today's {"Response": "DurationResponse"}, but it misbehaves the moment the file grows structure — and it fails open, producing a subtly wrong spec rather than an error.
perl is already a hard requirement and JSON::PP is core, so no new dependency:
done < <(perl -MJSON::PP -0777 -ne 'my $m = decode_json($_); print "$_\t$m->{$_}\n" for keys %$m' "$RENAMED_MODELS")|
|
||
| # Record which spec release the checked-in client was generated from, so a diff | ||
| # in `lib/src/generated/api` can always be traced back to a protocol tag. | ||
| if [[ -f "$API_BARREL_FILE" ]]; then |
There was a problem hiding this comment.
generate-client was just told to write into $OUTPUT_DIR_FEEDS, so a missing api.dart means generation failed — but this silently skips the stamp and the run still reports 🎉. Worth making it a hard error, the way the sumScores block below does.
| # Every spec release is tagged, so the newest reachable tag is the version the | ||
| # spec on the default branch belongs to. | ||
| SPEC_VERSION="${PROTOCOL_VERSION:-$(git -C "$PROTOCOL_GIT_DIR" describe --tags --abbrev=0 --match "$TAG_GLOB")}" | ||
| [[ -n "$SPEC_VERSION" ]] || { echo "❌ Could not resolve a '$TAG_GLOB' tag in the protocol repo"; exit 1; } |
There was a problem hiding this comment.
Nit: unreachable. Under set -e a failing command substitution aborts the assignment on the line above (set -euo pipefail; V="${X:-$(false)}" exits 1). Harmless, just doesn't do what it looks like.
2 participants
Submit a pull request
Closes FLU-
Closes #
CLA
Description of the pull request
Screenshots / Videos