ronantakizawa/codereview-bench huggingface dataset:
Repository: tokio-rs/axum
File: axum-extra/src/extract/cookie/private.rs (Rust)

Code before:
///     // that holds the key used to sign cookies
///     key: Key,
/// }
///
/// // this impl tells `SignedCookieJar` how to access the key from our state
/// impl FromRef<AppState> for Key {
///     fn from_ref(state: &AppState) -> Self {
///         state.key.clone()
///     }
/// }
///
/// let state = AppState {
///     // Generate a secure key
///     //
///     // You probably don't wanna generate a new one each time the app starts though
///     key: Key::generate(),
/// };
///
/// let app = Router::with_state(state)
///     .route("/set", post(set_secret))
///     .route("/get", get(get_secret));
/// # let app: Router<_> = app;
/// ```
///
/// If you have been using `Arc<AppState>` you cannot implement `FromRef<Arc<AppState>> for Key`.
/// You can use a new type instead: `struct AppState(Arc<InnerState>);` and implement `Deref`.
///
/// ```rust
/// #[derive(Clone)]
/// struct AppState(Arc<InnerState>);
///
/// // deref so you can still access the inner fields easily
/// impl Deref for AppState {
///     type Target = InnerState;
///
///     fn deref(&self) -> &Self::Target {
///         &*self.0
///     }
/// }
///
/// struct InnerState {
///     key: Key
/// }
///
/// impl FromRef<AppState> for Key {
///     fn from_ref(state: &AppState) -> Self {
///         state.0.key.clone()
///     }
/// }
/// ```
pub struct PrivateCookieJar<K = Key> {

Review comment (suggestion):
```suggestion
/// You can use a new type instead:
```

Code after:
///     // that holds the key used to sign cookies
///     key: Key,
/// }
///
/// // this impl tells `SignedCookieJar` how to access the key from our state
/// impl FromRef<AppState> for Key {
///     fn from_ref(state: &AppState) -> Self {
///         state.key.clone()
///     }
/// }
///
/// let state = AppState {
///     // Generate a secure key
///     //
///     // You probably don't wanna generate a new one each time the app starts though
///     key: Key::generate(),
/// };
///
/// let app = Router::with_state(state)
///     .route("/set", post(set_secret))
///     .route("/get", get(get_secret));
/// # let app: Router<_> = app;
/// ```
///
/// If you have been using `Arc<AppState>` you cannot implement `FromRef<Arc<AppState>> for Key`.
/// You can use a new type instead:
///
/// ```rust
/// # use axum::extract::FromRef;
/// # use axum_extra::extract::cookie::{PrivateCookieJar, Cookie, Key};
/// use std::sync::Arc;
/// use std::ops::Deref;
///
/// #[derive(Clone)]
/// struct AppState(Arc<InnerState>);
///
/// // deref so you can still access the inner fields easily
/// impl Deref for AppState {
///     type Target = InnerState;
///
///     fn deref(&self) -> &Self::Target {
///         &*self.0
///     }
/// }
///
/// struct InnerState {
///     key: Key
/// }
///
/// impl FromRef<AppState> for Key {
///     fn from_ref(state: &AppState) -> Self {
Repository: tokio-rs/axum
File: axum-extra/src/extract/cookie/signed.rs (Rust)

Code before:
/// struct AppState {
///     // that holds the key used to sign cookies
///     key: Key,
/// }
///
/// // this impl tells `SignedCookieJar` how to access the key from our state
/// impl FromRef<AppState> for Key {
///     fn from_ref(state: &AppState) -> Self {
///         state.key.clone()
///     }
/// }
///
/// let state = AppState {
///     // Generate a secure key
///     //
///     // You probably don't wanna generate a new one each time the app starts though
///     key: Key::generate(),
/// };
///
/// let app = Router::with_state(state)
///     .route("/sessions", post(create_session))
///     .route("/me", get(me));
/// # let app: Router<_> = app;
/// ```
/// If you have been using `Arc<AppState>` you cannot implement `FromRef<Arc<AppState>> for Key`.
/// You can use a new type instead: `struct AppState(Arc<InnerState>);` and implement `Deref`.
///
/// ```rust
/// #[derive(Clone)]
/// struct AppState(Arc<InnerState>);
///
/// // deref so you can still access the inner fields easily
/// impl Deref for AppState {
///     type Target = InnerState;
///
///     fn deref(&self) -> &Self::Target {
///         &*self.0
///     }
/// }
///
/// struct InnerState {
///     key: Key
/// }
///
/// impl FromRef<AppState> for Key {
///     fn from_ref(state: &AppState) -> Self {
///         state.0.key.clone()
///     }
/// }
/// ```
pub struct SignedCookieJar<K = Key> {

Review comment (suggestion):
```suggestion
///
/// If you have been using `Arc<AppState>` you cannot implement `FromRef<Arc<AppState>> for Key`.
/// You can use a new type instead:
```

Code after:
/// struct AppState {
///     // that holds the key used to sign cookies
///     key: Key,
/// }
///
/// // this impl tells `SignedCookieJar` how to access the key from our state
/// impl FromRef<AppState> for Key {
///     fn from_ref(state: &AppState) -> Self {
///         state.key.clone()
///     }
/// }
///
/// let state = AppState {
///     // Generate a secure key
///     //
///     // You probably don't wanna generate a new one each time the app starts though
///     key: Key::generate(),
/// };
///
/// let app = Router::with_state(state)
///     .route("/sessions", post(create_session))
///     .route("/me", get(me));
/// # let app: Router<_> = app;
/// ```
/// If you have been using `Arc<AppState>` you cannot implement `FromRef<Arc<AppState>> for Key`.
/// You can use a new type instead:
///
/// ```rust
/// # use axum::extract::FromRef;
/// # use axum_extra::extract::cookie::{PrivateCookieJar, Cookie, Key};
/// use std::sync::Arc;
/// use std::ops::Deref;
///
/// #[derive(Clone)]
/// struct AppState(Arc<InnerState>);
///
/// // deref so you can still access the inner fields easily
/// impl Deref for AppState {
///     type Target = InnerState;
///
///     fn deref(&self) -> &Self::Target {
///         &*self.0
///     }
/// }
///
/// struct InnerState {
///     key: Key
/// }
///
/// impl FromRef<AppState> for Key {
///     fn from_ref(state: &AppState) -> Self {
Repository: tokio-rs/axum
File: axum-core/src/extract/from_ref.rs (Rust)

Code before:
/// Used to do reference-to-value conversions thus not consuming the input value.
///
/// This is mainly used with [`State`] to extract "substates" from a reference to main application
/// state.
///
/// See [`State`] for more details on how library authors should use this trait.
///
/// This trait can be derived using `#[derive(axum_macros::FromRef)]`.
///
/// [`State`]: https://docs.rs/axum/0.6/axum/extract/struct.State.html
/// [`#[derive(axum_macros::FromRef)]`]: https://docs.rs/axum-macros/latest/axum_macros/derive.FromRef.html
// NOTE: This trait is defined in axum-core, even though it is mainly used with `State` which is
// defined in axum. That allows crate authors to use it when implementing extractors.
pub trait FromRef<T> {
    /// Converts to this type from a reference to the input type.
    fn from_ref(input: &T) -> Self;
}

impl<T> FromRef<T> for T
where
    T: Clone,
{
    fn from_ref(input: &T) -> Self {
        input.clone()
    }
}


Review comment (suggestion):
The link on this didn't work for some reason but I don't actually think we need it since the macro is exported at the same path as `FromRef` itself.

Code after:
/// Used to do reference-to-value conversions thus not consuming the input value.
///
/// This is mainly used with [`State`] to extract "substates" from a reference to main application
/// state.
///
/// See [`State`] for more details on how library authors should use this trait.
///
/// This trait can be derived using `#[derive(FromRef)]`.
///
/// [`State`]: https://docs.rs/axum/0.6/axum/extract/struct.State.html
// NOTE: This trait is defined in axum-core, even though it is mainly used with `State` which is
// defined in axum. That allows crate authors to use it when implementing extractors.
pub trait FromRef<T> {
    /// Converts to this type from a reference to the input type.
    fn from_ref(input: &T) -> Self;
}

impl<T> FromRef<T> for T
where
    T: Clone,
{
    fn from_ref(input: &T) -> Self {
        input.clone()
    }
}
Repository: tokio-rs/axum
File: examples/websockets/src/main.rs (Rust)

Code before:
fn process_message(msg: Message, who: SocketAddr) -> bool {
    match msg {
        Message::Text(t) => {
            println!(">>> {} sent str: {:?}", who, t);
        }
        Message::Binary(d) => {
            println!(">>> {} sent {} bytes: {:?}", who, d.len(), d);
        }
        Message::Close(c) => {
            if let Some(cf) = c {
                println!(
                    ">>> {} sent close with code {} and reason `{}`",
                    who, cf.code, cf.reason
                );
            } else {
                println!(">>> {} somehow sent close message without CloseFrame", who);
            }
            return true;
        }

        Message::Pong(v) => {
            println!(">>> {} sent pong with {:?}", who, v);
        }
        // You should never need to manually handle these, as tungstenite websocket library
        // will do so for you automagically by replying with Pong and copying the v according to
        // spec. But if you need the contents of the pings you can see them here.
        Message::Ping(v) => {
            println!(">>> {} sent ping with {:?}", who, v);
        }
    }
    return false;
}


Review comment (suggestion):
Either we should explain what tungstenite is (the websocket library axum uses internally) or say that axum handles pings.

Code after:
    println!("Websocket context {} destroyed", who);
}

/// helper to print contents of messages to stdout. Has special treatment for Close.
fn process_message(msg: Message, who: SocketAddr) -> ControlFlow<(), ()> {
    match msg {
        Message::Text(t) => {
            println!(">>> {} sent str: {:?}", who, t);
        }
        Message::Binary(d) => {
            println!(">>> {} sent {} bytes: {:?}", who, d.len(), d);
        }
        Message::Close(c) => {
            if let Some(cf) = c {
                println!(
                    ">>> {} sent close with code {} and reason `{}`",
                    who, cf.code, cf.reason
                );
            } else {
                println!(">>> {} somehow sent close message without CloseFrame", who);
            }
            return ControlFlow::Break(());
        }

        Message::Pong(v) => {
            println!(">>> {} sent pong with {:?}", who, v);
        }
        // You should never need to manually handle Message::Ping, as axum's websocket library
        // will do so for you automagically by replying with Pong and copying the v according to
        // spec. But if you need the contents of the pings you can see them here.
        Message::Ping(v) => {
            println!(">>> {} sent ping with {:?}", who, v);
        }
    }
    ControlFlow::Continue(())
}
Repository: tokio-rs/axum
File: examples/websockets/src/main.rs (Rust)

Code before:
//! Example websocket server.
//!
//! Run the server with
//!
//! ```not_rust
//! cargo run -p example-websockets
//! firefox http://localhost:3000
//! ```
//!
//! Alternatively you can run rust client with
//! ```not_rust
//! cargo run -p example-client
//! ```

use axum::{
    extract::{
        ws::{Message, WebSocket, WebSocketUpgrade},
        TypedHeader,
    },
    http::StatusCode,
    response::IntoResponse,
    routing::{get, get_service},
    Router,
};

use std::borrow::Cow;
use std::{net::SocketAddr, path::PathBuf};
use tower_http::{
    services::ServeDir,
    trace::{DefaultMakeSpan, TraceLayer},
};

use tracing_subscriber::{layer::SubscriberExt, util::SubscriberInitExt};

//allows to extract the IP of connecting user

Review comment (suggestion):
```suggestion
//! Alternatively you can run the rust client with
```

Code after:
//! Example websocket server.
//!
//! Run the server with
//!
//! ```not_rust
//! cargo run -p example-websockets
//! firefox http://localhost:3000
//! ```
//!
//! Alternatively you can run the rust client with
//! ```not_rust
//! cargo run -p example-client
//! ```

use axum::{
    extract::{
        ws::{Message, WebSocket, WebSocketUpgrade},
        TypedHeader,
    },
    http::StatusCode,
    response::IntoResponse,
    routing::{get, get_service},
    Router,
};

use std::borrow::Cow;
use std::ops::ControlFlow;
use std::{net::SocketAddr, path::PathBuf};
use tower_http::{
    services::ServeDir,
    trace::{DefaultMakeSpan, TraceLayer},
};

use tracing_subscriber::{layer::SubscriberExt, util::SubscriberInitExt};
Repository: tokio-rs/axum
File: examples/websockets/src/client.rs (Rust)

Code before:
//! Based on tokio-tungstenite example websocket client, but with multiple
//! concurrent websocket clients in one package
//!
//! This example will connect to a server specified in the argument in the specified
//! number of threads, and then flood some test messages over websocket.
//! This will also print whatever it gets into stdout.
//!
//! Note that this is not currently optimized for performance, especially around
//! stdout mutex management. Rather it's intended to show an example of working with a
//! websocket server.
//!

//boilerplate
use clap::Parser;
use std::borrow::Cow;
use std::time::Instant;

//we need tungstenite for websocket impl (same library as what axum is using)
use tokio::task::JoinHandle;
use tokio_tungstenite::tungstenite::protocol::frame::coding::CloseCode;
use tokio_tungstenite::tungstenite::protocol::CloseFrame;
use tokio_tungstenite::{connect_async, tungstenite::protocol::Message};
use url::Url;
//stream splitting
use futures_util::{SinkExt, StreamExt};


Review comment (suggestion):
Hm I'm kinda torn on having a client example here. Like what is it trying to demonstrate? How to write a websocket client in rust? Maybe just linking to [tungstenite's examples](https://github.com/snapview/tungstenite-rs/tree/master/examples) is enough for that. I generally prefer axum's examples to focus on axum related things, which I'd argue this isn't.

Code after:
//! Based on tokio-tungstenite example websocket client, but with multiple
//! concurrent websocket clients in one package
//!
//! This will connect to a server specified in the SERVER with N_CLIENTS
//! concurrent connections, and then flood some test messages over websocket.
//! This will also print whatever it gets into stdout.
//!
//! Note that this is not currently optimized for performance, especially around
//! stdout mutex management. Rather it's intended to show an example of working with axum's
//! websocket server and how the client-side and server-side code can be quite similar.
//!

use futures_util::stream::FuturesUnordered;
use futures_util::{SinkExt, StreamExt};
use std::borrow::Cow;
use std::ops::ControlFlow;
use std::time::Instant;

// we will use tungstenite for websocket client impl (same library as what axum is using)
use tokio_tungstenite::{
    connect_async,
    tungstenite::protocol::{frame::coding::CloseCode, CloseFrame, Message},
};

const N_CLIENTS: usize = 2; //set to desired number
const SERVER: &'static str = "ws://127.0.0.1:3000/ws";
Repository: tokio-rs/axum
File: axum/src/middleware/from_fn.rs (Rust)

Code before:
use axum_core::extract::{FromRequest, FromRequestParts};
use futures_util::future::BoxFuture;
use http::Request;
use std::{
    any::type_name,
    convert::Infallible,
    fmt,
    future::Future,
    marker::PhantomData,
    pin::Pin,
    task::{Context, Poll},
};
use tower::{util::BoxCloneService, ServiceBuilder};
use tower_layer::Layer;
use tower_service::Service;

/// Create a middleware from an async function.
///
/// `from_fn` requires the function given to
///
/// 1. Be an `async fn`.
/// 2. Take one or more [extractors] as the first arguments.
/// 3. Take [`Next<B>`](Next) as the final argument.
/// 4. Return something that implements [`IntoResponse`].
///
/// Note that this function doesn't support extracting [`State`]. For that [`from_fn_with_state`].
///
/// # Example
///
/// ```rust
/// use axum::{
///     Router,
///     http::{self, Request},
///     routing::get,
///     response::Response,
///     middleware::{self, Next},
/// };
///
/// async fn my_middleware<B>(
///     request: Request<B>,
///     next: Next<B>,
/// ) -> Response {
///     // do something with `request`...
///
///     let response = next.run(request).await;
///
///     // do something with `response`...
///
///     response
/// }
///

Review comment (refactor):
```suggestion
/// Note that this function doesn't support extracting [`State`]. For that, use [`from_fn_with_state`].
```

Code after:
use axum_core::extract::{FromRequest, FromRequestParts};
use futures_util::future::BoxFuture;
use http::Request;
use std::{
    any::type_name,
    convert::Infallible,
    fmt,
    future::Future,
    marker::PhantomData,
    pin::Pin,
    task::{Context, Poll},
};
use tower::{util::BoxCloneService, ServiceBuilder};
use tower_layer::Layer;
use tower_service::Service;

/// Create a middleware from an async function.
///
/// `from_fn` requires the function given to
///
/// 1. Be an `async fn`.
/// 2. Take one or more [extractors] as the first arguments.
/// 3. Take [`Next<B>`](Next) as the final argument.
/// 4. Return something that implements [`IntoResponse`].
///
/// Note that this function doesn't support extracting [`State`]. For that, use [`from_fn_with_state`].
///
/// # Example
///
/// ```rust
/// use axum::{
///     Router,
///     http::{self, Request},
///     routing::get,
///     response::Response,
///     middleware::{self, Next},
/// };
///
/// async fn my_middleware<B>(
///     request: Request<B>,
///     next: Next<B>,
/// ) -> Response {
///     // do something with `request`...
///
///     let response = next.run(request).await;
///
///     // do something with `response`...
///
///     response
/// }
///
Repository: tokio-rs/axum
File: examples/chat/src/main.rs (Rust)

Code before:
//! ```not_rust
//! cd examples && cargo run -p example-chat
//! ```

use axum::{
    extract::{
        ws::{Message, WebSocket, WebSocketUpgrade},
        State,
    },
    response::{Html, IntoResponse},
    routing::get,
    Router,
};
use futures::{sink::SinkExt, stream::StreamExt};
use std::{
    collections::HashSet,
    net::SocketAddr,
    sync::{Arc, Mutex},
};
use tokio::sync::broadcast;
use tracing_subscriber::{layer::SubscriberExt, util::SubscriberInitExt};

// Our shared state
struct AppState {
    user_set: Mutex<HashSet<String>>, // Currently taken user names (must be unique)
    tx: broadcast::Sender<String>, // Receivers can be created via tx.subscribe()
}

#[tokio::main]
async fn main() {
    tracing_subscriber::registry()
        .with(
            tracing_subscriber::EnvFilter::try_from_default_env()
                .unwrap_or_else(|_| "example_chat=trace".into()),
        )
        .with(tracing_subscriber::fmt::layer())
        .init();

    // Set up application state for use with with_state():
    let user_set = Mutex::new(HashSet::new());
    let (tx, _rx) = broadcast::channel(100);

    let app_state = Arc::new(AppState { user_set, tx });

    let app = Router::new()
        .route("/", get(index))
        .route("/websocket", get(websocket_handler))
        .with_state(app_state);

    let addr = SocketAddr::from(([127, 0, 0, 1], 3000));
    tracing::debug!("listening on {}", addr);

Review comment (suggestion):
```suggestion
    // We require unique usernames. This tracks which usernames have been taken.
    user_set: Mutex<HashSet<String>>,
    // Channel used to send messages to all connected clients.
    tx: broadcast::Sender<String>,
```

Code after:
//! ```not_rust
//! cd examples && cargo run -p example-chat
//! ```

use axum::{
    extract::{
        ws::{Message, WebSocket, WebSocketUpgrade},
        State,
    },
    response::{Html, IntoResponse},
    routing::get,
    Router,
};
use futures::{sink::SinkExt, stream::StreamExt};
use std::{
    collections::HashSet,
    net::SocketAddr,
    sync::{Arc, Mutex},
};
use tokio::sync::broadcast;
use tracing_subscriber::{layer::SubscriberExt, util::SubscriberInitExt};

// Our shared state
struct AppState {
    // We require unique usernames. This tracks which usernames have been taken.
    user_set: Mutex<HashSet<String>>,
    // Channel used to send messages to all connected clients.
    tx: broadcast::Sender<String>,
}

#[tokio::main]
async fn main() {
    tracing_subscriber::registry()
        .with(
            tracing_subscriber::EnvFilter::try_from_default_env()
                .unwrap_or_else(|_| "example_chat=trace".into()),
        )
        .with(tracing_subscriber::fmt::layer())
        .init();

    // Set up application state for use with with_state().
    let user_set = Mutex::new(HashSet::new());
    let (tx, _rx) = broadcast::channel(100);

    let app_state = Arc::new(AppState { user_set, tx });

    let app = Router::new()
        .route("/", get(index))
        .route("/websocket", get(websocket_handler))
        .with_state(app_state);
Repository: tokio-rs/axum
File: axum-extra/src/routing/typed.rs (Rust)

Code before:
    }

    impl TypedPath for UsersShow {
        const PATH: &'static str = "/users/:id";
    }

    #[derive(Serialize)]
    struct Params {
        foo: &'static str,
        bar: i32,
        baz: bool,
    }

    #[test]
    fn with_params() {
        let path = UsersShow { id: 1 }.with_query_params(Params {
            foo: "foo",
            bar: 123,
            baz: true,
        });

        let uri = path.to_uri();

        // according to [the spec] starting the params with `?&` is allowed specifically:
        //
        // > If bytes is the empty byte sequence, then continue.
        //
        // [the spec]: https://url.spec.whatwg.org/#urlencoded-parsing
        assert_eq!(uri, "/users/1?&foo=foo&bar=123&baz=true");
    }

    #[test]
    fn with_params_called_multiple_times() {
        let path = UsersShow { id: 1 }
            .with_query_params(Params {
                foo: "foo",
                bar: 123,
                baz: true,
            })
            .with_query_params([("qux", 1337)]);

        let uri = path.to_uri();

        assert_eq!(uri, "/users/1?&foo=foo&bar=123&baz=true&qux=1337");
    }
}


Review comment (suggestion):
We should also address how query params are merged? EG:
```rust
 let path = UsersShow { id: 1 }
            .with_query_params(Params {
                foo: "foo",
                bar: 123,
                baz: true,
            })
            .with_query_params([("foo", 1337)]);
            // does this override `foo` or get merged into a `foo[]="foo"&foo[]=1337`?
```

Code after:
    struct Params {
        foo: &'static str,
        bar: i32,
        baz: bool,
    }

    #[test]
    fn with_params() {
        let path = UsersShow { id: 1 }.with_query_params(Params {
            foo: "foo",
            bar: 123,
            baz: true,
        });

        let uri = path.to_uri();

        // according to [the spec] starting the params with `?&` is allowed specifically:
        //
        // > If bytes is the empty byte sequence, then continue.
        //
        // [the spec]: https://url.spec.whatwg.org/#urlencoded-parsing
        assert_eq!(uri, "/users/1?&foo=foo&bar=123&baz=true");
    }

    #[test]
    fn with_params_called_multiple_times() {
        let path = UsersShow { id: 1 }
            .with_query_params(Params {
                foo: "foo",
                bar: 123,
                baz: true,
            })
            .with_query_params([("qux", 1337)]);

        let uri = path.to_uri();

        assert_eq!(uri, "/users/1?&foo=foo&bar=123&baz=true&qux=1337");
    }
}