Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds/expands crate documentation intended for docs.rs and contributor/agent guidance, primarily via Rustdoc comments and new markdown docs.
Changes:
- Expanded Rustdoc module/function documentation across core SDK and ProxyWasm modules, plus the derive macro crate.
- Added repository-level documentation files (
DOCUMENTATION.md,AGENTS.md) and improved crate metadata for docs.rs/crates.io. - Updated
Cargo.tomlmetadata (readme/keywords/categories + docs.rs settings) for both the main crate andfastedge-derive.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/proxywasm/utils.rs | Added detailed Rustdoc for diagnostics helper. |
| src/proxywasm/secret.rs | Added Rustdoc describing secret access and examples. |
| src/proxywasm/mod.rs | Added module-level Rustdoc describing ProxyWasm layer. |
| src/proxywasm/dictionary.rs | Added Rustdoc for dictionary lookups and behavior. |
| src/lib.rs | Major crate-level and module-level Rustdoc expansion + examples. |
| src/http_client.rs | Added Rustdoc for send_request and usage examples. |
| src/helper.rs | Added Rustdoc describing internal list deserialization format. |
| derive/src/lib.rs | Added crate/macro Rustdoc and usage examples. |
| derive/README.md | Reworked README to match #[fastedge::http] usage and current APIs. |
| derive/Cargo.toml | Added crate metadata + docs.rs config for the derive crate. |
| DOCUMENTATION.md | Added long-form “complete documentation” markdown. |
| Cargo.toml | Added package metadata + docs.rs build configuration. |
| AGENTS.md | Added AI agent guide / project overview and conventions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ```rust | ||
| use fastedge::body::Body; | ||
| use fastedge::http::{header, Method, Request, Response, StatusCode}; | ||
| use pulldown_cmark::{Options, Parser}; | ||
| use std::env; | ||
|
|
||
| #[fastedge::http] | ||
| fn main(req: Request<Body>) -> Result<Response<Body>, http::Error> { |
There was a problem hiding this comment.
This example uses http::Error and mime::TEXT_HTML, but those crates are not accessible unless the user adds http/mime as direct dependencies (transitive deps from fastedge can’t be imported). Since fastedge re-exports http as fastedge::http, consider using fastedge::http::Error (or anyhow::Result) and avoid mime::... here (e.g., use a literal Content-Type) or explicitly document the extra Cargo.toml dependencies required for this snippet.
| /// - 4 bytes: number of items (little-endian u32) | ||
| /// - For each item: 4 bytes for size (little-endian u32) | ||
| /// - Followed by the actual data for all items |
There was a problem hiding this comment.
The documented wire format doesn’t match the implementation: the serializer/test adds a trailing 0 byte after each item, and the header lengths are written as 32-bit little-endian integers (currently via i32::to_le_bytes). The docs currently omit the per-item terminator and describe the sizes as u32 without mentioning the terminator; please update the doc comment (or the format) so it accurately describes what deserialize_list expects.
| /// - 4 bytes: number of items (little-endian u32) | |
| /// - For each item: 4 bytes for size (little-endian u32) | |
| /// - Followed by the actual data for all items | |
| /// - 4 bytes: number of items (little-endian 32-bit integer) | |
| /// - For each item: 4 bytes for the item size in bytes (little-endian 32-bit integer) | |
| /// - Then, for each item in order: `size` bytes of item data followed by a single trailing `0x00` byte |
| /// # Returns | ||
| /// | ||
| /// Returns `Ok(Some(value))` if the secret exists, `Ok(None)` if not found, | ||
| /// or `Err(status)` on failure. | ||
| /// |
There was a problem hiding this comment.
This doc says the function can return Err(status) on failure, but the implementation never returns Err—it panics for any status other than 0 or 1. Either adjust the docs to describe the panic behavior, or (preferably) change the status => ... arm to return Err(status) to match the signature.
| /// # Returns | ||
| /// | ||
| /// Returns `Ok(Some(value))` if a secret was effective at that time, | ||
| /// `Ok(None)` if no secret was configured, or `Err(status)` on failure. | ||
| /// |
There was a problem hiding this comment.
Same mismatch here: the docs mention Err(status) on failure, but get_effective_at panics on any unexpected status instead of returning Err(status). Please align the docs and implementation (returning Err(status) would fit the current return type).
| /// # Errors | ||
| /// | ||
| /// Returns an error if: | ||
| /// - The HTTP method is not supported | ||
| /// - The request is malformed | ||
| /// - The backend service is unreachable | ||
| /// - The response is invalid |
There was a problem hiding this comment.
The # Errors section implies normal error returns, but send_request can panic if any request header value is not valid UTF-8 because header conversion uses value.to_str().unwrap() in TryFrom<(&Parts, &Body)>. Either handle invalid UTF-8 header values as an Err (recommended) or document this limitation explicitly in the docs.
2 participants
AI-generated docs for docs.rs (in comments) and AGENTS.md