Skip to content

Fix Rust SDK review issues#1492

Merged
ChiragAgg5k merged 1 commit intomasterfrom
fix/rust-sdk-review-comments
May 1, 2026
Merged

Fix Rust SDK review issues#1492
ChiragAgg5k merged 1 commit intomasterfrom
fix/rust-sdk-review-comments

Conversation

@ChiragAgg5k
Copy link
Copy Markdown
Member

@ChiragAgg5k ChiragAgg5k commented May 1, 2026

What

  • Fix Rust docs examples for nullable parameters so generated examples pass Some(...) when the Rust method signature expects Option<T>.
  • Relax Rust url, idna, and idna_adapter dependencies from exact pins to bounded ranges that still respect Rust 1.83 compatibility.

Why

Greptile flagged Rust SDK release review issues in appwrite/sdk-for-rust#13: optional policy examples did not compile, and exact dependency pins were too strict for a library crate.

The email-template PATCH route was checked against the current Appwrite route/spec and still correctly uses /project/templates/email with templateId in the body. The password-history typo is left untouched because it comes from the upstream spec.

Testing

  • php example.php rust
  • composer lint-twig
  • vendor/bin/phpunit tests/Rust183Test.php

@ChiragAgg5k ChiragAgg5k force-pushed the fix/rust-sdk-review-comments branch from 3ca1187 to 36906d2 Compare May 1, 2026 15:09
@ChiragAgg5k ChiragAgg5k merged commit 24e6c0f into master May 1, 2026
57 checks passed
@ChiragAgg5k ChiragAgg5k deleted the fix/rust-sdk-review-comments branch May 1, 2026 15:11
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 1, 2026

Greptile Summary

This PR makes two targeted fixes to the Rust SDK generator: it corrects getDocsArgumentExample in Rust.php so that required-but-nullable parameters are wrapped in Some(...) (matching the Option<T> type the Twig template already generates for those params), and it relaxes exact-pinned url, idna, and idna_adapter dependencies in Cargo.toml.twig to bounded ranges. Both changes are clearly motivated and logically consistent with the existing code.

Confidence Score: 4/5

Safe to merge; only a minor style concern about the upper version bound on url in the template.

The nullable-param fix is correct and directly aligned with the Twig template's type generation logic. The dependency relaxation is a clear improvement over exact pins. The sole observation is a P2 style note about the url <2.6 upper bound being potentially overly restrictive for downstream library consumers.

No files require special attention beyond the P2 note on templates/rust/Cargo.toml.twig.

Important Files Changed

Filename Overview
src/SDK/Language/Rust.php Fixes getDocsArgumentExample to wrap required+nullable param values in Some(...), matching the Twig template's Option<T> signature generation for those params.
templates/rust/Cargo.toml.twig Relaxes url, idna, and idna_adapter from exact pins to bounded ranges; upper bounds could be marginally too tight for a library crate but are defensible given the Rust 1.83 MSRV.

Reviews (1): Last reviewed commit: "Fix Rust SDK review issues" | Re-trigger Greptile

url = "=2.5.4"
idna = "=1.0.3"
idna_adapter = "=1.0.0"
url = ">=2.5.4, <2.6"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Upper bound may be unnecessarily restrictive for a library crate

url = ">=2.5.4, <2.6" (equivalent to ~2.5.4) pins the SDK to the 2.5.x patch series. If a downstream consumer's dependency graph resolves url to a 2.6.x release, it will conflict. For a library crate the idiomatic Rust approach is to use the widest range compatible with the stated MSRV — here that would be url = "2" or at minimum url = ">=2.5.4" — and let the top-level binary crate perform any tighter pinning via Cargo.lock. The rust-version = "1.83" field in the same file already communicates the compiler floor; no upper dependency bound is needed to enforce it unless a specific url 2.6 release is known to raise the MSRV above 1.83.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

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