ronantakizawa/codereview-bench huggingface dataset:
Repository: actix/actix-web
File: src/middleware/condition.rs (Rust)

Code before:
use actix_service::{Service, Transform};
use futures::future::{ok, Either, FutureResult, Map};
use futures::{Future, Poll};

/// `Middleware` for conditianally enables another middleware
/// The controled middleware must not change the `Service` interfaces.
/// This means you cannot control such middlewares like `Logger` or `Compress`.
///
/// ## Usage
///
/// ```
/// use actix_web::middleware::{Condition, NormalizePath};
/// use actix_web::App;
///
/// fn main() {
///     let enable_normalize = std::env::var("NORMALIZE_PATH") == Ok("true".into());
///     let app = App::new()
///         .wrap(Condition::new(enable_normalize, NormalizePath));
/// }
/// ```
///
pub struct Condition<T> {
    trans: T,
    enable: bool,
}

impl<T> Condition<T> {
    pub fn new(enable: bool, trans: T) -> Self {
        Self { trans, enable }
    }
}

impl<S, T> Transform<S> for Condition<T>
where
    S: Service,
    T: Transform<S, Request = S::Request, Response = S::Response, Error = S::Error>,

Review comment (suggestion):
IMO: We have explicit `rust` annotation in other places so it seems reasonable to add it here.
```suggestion
/// ```rust
```

Code after:
//! `Middleware` for conditionally enables another middleware.
use actix_service::{Service, Transform};
use futures::future::{ok, Either, FutureResult, Map};
use futures::{Future, Poll};

/// `Middleware` for conditionally enables another middleware.
/// The controled middleware must not change the `Service` interfaces.
/// This means you cannot control such middlewares like `Logger` or `Compress`.
///
/// ## Usage
///
/// ```rust
/// use actix_web::middleware::{Condition, NormalizePath};
/// use actix_web::App;
///
/// fn main() {
///     let enable_normalize = std::env::var("NORMALIZE_PATH") == Ok("true".into());
///     let app = App::new()
///         .wrap(Condition::new(enable_normalize, NormalizePath));
/// }
/// ```
pub struct Condition<T> {
    trans: T,
    enable: bool,
}

impl<T> Condition<T> {
    pub fn new(enable: bool, trans: T) -> Self {
        Self { trans, enable }
    }
}

impl<S, T> Transform<S> for Condition<T>
where
    S: Service,
    T: Transform<S, Request = S::Request, Response = S::Response, Error = S::Error>,
Repository: actix/actix-web
File: actix-files/src/named.rs (Rust)

Code before:
use actix_web::http::header::{
    self, ContentDisposition, DispositionParam, DispositionType,
};
use actix_web::http::{ContentEncoding, Method, StatusCode};
use actix_web::middleware::BodyEncoding;
use actix_web::{Error, HttpMessage, HttpRequest, HttpResponse, Responder};
use actix_web::guard::Guard;

use crate::range::HttpRange;
use crate::ChunkedReadFile;

bitflags! {
    pub(crate) struct Flags: u8 {
        const ETAG = 0b0000_0001;
        const LAST_MD = 0b0000_0010;
        const CONTENT_DISPOSITION = 0b0000_0100;
    }
}

impl Default for Flags {
    fn default() -> Self {
        Flags::all()
    }
}

/// A file with an associated name.
pub struct NamedFile {
    path: PathBuf,
    file: File,
    modified: Option<SystemTime>,
    pub(crate) md: Metadata,
    pub(crate) flags: Flags,
    pub(crate) status_code: StatusCode,
    pub(crate) content_type: mime::Mime,
    pub(crate) content_disposition: header::ContentDisposition,
    pub(crate) encoding: Option<ContentEncoding>,
    pub(crate) guard: Option<Box<dyn Guard>>,
}

impl NamedFile {
    /// Creates an instance from a previously opened file.
    ///
    /// The given `path` need not exist and is only used to determine the `ContentType` and
    /// `ContentDisposition` headers.
    ///
    /// # Examples
    ///
    /// ```rust
    /// use actix_files::NamedFile;
    /// use std::io::{self, Write};
    /// use std::env;

Review comment (bug):
I don't think it's a good idea to remove the Debug implementation.

Code after:
use actix_web::http::header::{
    self, ContentDisposition, DispositionParam, DispositionType,
};
use actix_web::http::{ContentEncoding, StatusCode};
use actix_web::middleware::BodyEncoding;
use actix_web::{Error, HttpMessage, HttpRequest, HttpResponse, Responder};

use crate::range::HttpRange;
use crate::ChunkedReadFile;

bitflags! {
    pub(crate) struct Flags: u8 {
        const ETAG = 0b0000_0001;
        const LAST_MD = 0b0000_0010;
        const CONTENT_DISPOSITION = 0b0000_0100;
    }
}

impl Default for Flags {
    fn default() -> Self {
        Flags::all()
    }
}

/// A file with an associated name.
#[derive(Debug)]
pub struct NamedFile {
    path: PathBuf,
    file: File,
    modified: Option<SystemTime>,
    pub(crate) md: Metadata,
    pub(crate) flags: Flags,
    pub(crate) status_code: StatusCode,
    pub(crate) content_type: mime::Mime,
    pub(crate) content_disposition: header::ContentDisposition,
    pub(crate) encoding: Option<ContentEncoding>,
}

impl NamedFile {
    /// Creates an instance from a previously opened file.
    ///
    /// The given `path` need not exist and is only used to determine the `ContentType` and
    /// `ContentDisposition` headers.
    ///
    /// # Examples
    ///
    /// ```rust
    /// use actix_files::NamedFile;
    /// use std::io::{self, Write};
    /// use std::env;
    /// use std::fs::File;
Repository: actix/actix-web
File: actix-files/src/named.rs (Rust)

Code before:
    pub fn set_content_encoding(mut self, enc: ContentEncoding) -> Self {
        self.encoding = Some(enc);
        self
    }

    #[inline]
    ///Specifies whether to use ETag or not.
    ///
    ///Default is true.
    pub fn use_etag(mut self, value: bool) -> Self {
        self.flags.set(Flags::ETAG, value);
        self
    }

    #[inline]
    ///Specifies whether to use Last-Modified or not.
    ///
    ///Default is true.
    pub fn use_last_modified(mut self, value: bool) -> Self {
        self.flags.set(Flags::LAST_MD, value);
        self
    }

    ///Specifies custom guards.
    /// 
    ///Default guards allow GET, HEAD. 
    pub fn use_guards<G: Guard + 'static>(mut self, guards: G) -> Self {
        self.guard = Some(Box::new(guards));
        self
    }

    pub(crate) fn etag(&self) -> Option<header::EntityTag> {
        // This etag format is similar to Apache's.
        self.modified.as_ref().map(|mtime| {
            let ino = {
                #[cfg(unix)]
                {
                    self.md.ino()
                }
                #[cfg(not(unix))]
                {
                    0
                }
            };

            let dur = mtime
                .duration_since(UNIX_EPOCH)
                .expect("modification time must be after epoch");
            header::EntityTag::strong(format!(
                "{:x}:{:x}:{:x}:{:x}",
                ino,

Review comment (suggestion):
```suggestion
    /// Default guards allow GET, HEAD. 
```

Code after:
        self
    }

    #[inline]
    ///Specifies whether to use ETag or not.
    ///
    ///Default is true.
    pub fn use_etag(mut self, value: bool) -> Self {
        self.flags.set(Flags::ETAG, value);
        self
    }

    #[inline]
    ///Specifies whether to use Last-Modified or not.
    ///
    ///Default is true.
    pub fn use_last_modified(mut self, value: bool) -> Self {
        self.flags.set(Flags::LAST_MD, value);
        self
    }

    pub(crate) fn etag(&self) -> Option<header::EntityTag> {
        // This etag format is similar to Apache's.
        self.modified.as_ref().map(|mtime| {
            let ino = {
                #[cfg(unix)]
                {
                    self.md.ino()
                }
                #[cfg(not(unix))]
                {
                    0
                }
            };

            let dur = mtime
                .duration_since(UNIX_EPOCH)
                .expect("modification time must be after epoch");
            header::EntityTag::strong(format!(
                "{:x}:{:x}:{:x}:{:x}",
                ino,
                self.md.len(),
                dur.as_secs(),
                dur.subsec_nanos()
            ))
        })
    }

    pub(crate) fn last_modified(&self) -> Option<header::HttpDate> {
        self.modified.map(|mtime| mtime.into())
    }
Repository: actix/actix-web
File: actix-files/src/named.rs (Rust)

Code before:
    /// Set content encoding for serving this file
    #[inline]
    pub fn set_content_encoding(mut self, enc: ContentEncoding) -> Self {
        self.encoding = Some(enc);
        self
    }

    #[inline]
    ///Specifies whether to use ETag or not.
    ///
    ///Default is true.
    pub fn use_etag(mut self, value: bool) -> Self {
        self.flags.set(Flags::ETAG, value);
        self
    }

    #[inline]
    ///Specifies whether to use Last-Modified or not.
    ///
    ///Default is true.
    pub fn use_last_modified(mut self, value: bool) -> Self {
        self.flags.set(Flags::LAST_MD, value);
        self
    }

    ///Specifies custom guards.
    /// 
    ///Default guards allow GET, HEAD. 
    pub fn use_guards<G: Guard + 'static>(mut self, guards: G) -> Self {
        self.guard = Some(Box::new(guards));
        self
    }

    pub(crate) fn etag(&self) -> Option<header::EntityTag> {
        // This etag format is similar to Apache's.
        self.modified.as_ref().map(|mtime| {
            let ino = {
                #[cfg(unix)]
                {
                    self.md.ino()
                }
                #[cfg(not(unix))]
                {
                    0
                }
            };

            let dur = mtime
                .duration_since(UNIX_EPOCH)
                .expect("modification time must be after epoch");
            header::EntityTag::strong(format!(

Review comment (suggestion):
```suggestion
    /// Specifies custom guards.
```

Code after:
    pub fn set_content_encoding(mut self, enc: ContentEncoding) -> Self {
        self.encoding = Some(enc);
        self
    }

    #[inline]
    ///Specifies whether to use ETag or not.
    ///
    ///Default is true.
    pub fn use_etag(mut self, value: bool) -> Self {
        self.flags.set(Flags::ETAG, value);
        self
    }

    #[inline]
    ///Specifies whether to use Last-Modified or not.
    ///
    ///Default is true.
    pub fn use_last_modified(mut self, value: bool) -> Self {
        self.flags.set(Flags::LAST_MD, value);
        self
    }

    pub(crate) fn etag(&self) -> Option<header::EntityTag> {
        // This etag format is similar to Apache's.
        self.modified.as_ref().map(|mtime| {
            let ino = {
                #[cfg(unix)]
                {
                    self.md.ino()
                }
                #[cfg(not(unix))]
                {
                    0
                }
            };

            let dur = mtime
                .duration_since(UNIX_EPOCH)
                .expect("modification time must be after epoch");
            header::EntityTag::strong(format!(
                "{:x}:{:x}:{:x}:{:x}",
                ino,
                self.md.len(),
                dur.as_secs(),
                dur.subsec_nanos()
            ))
        })
    }

    pub(crate) fn last_modified(&self) -> Option<header::HttpDate> {
Repository: actix/actix-web
File: actix-http/examples/echo.rs (Rust)

Code before:
        .bind("echo", "127.0.0.1:8080", || {
            HttpService::build()
                .client_timeout(1000)
                .client_disconnect(1000)
                .finish(|mut req: Request| {
                    async move {
                        let mut body = BytesMut::new();
                        while let Some(item) = req.payload().next().await {
                            body.extend_from_slice(&item?);
                        }

                        info!("request body: {:?}", body);
                        Ok::<_, Error>(
                            Response::Ok()
                                .header(
                                    "x-head",
                                    HeaderValue::from_static("dummy value!"),
                                )
                                .body(body),
                        )
                    }
                })
                .tcp()
        })?
        .run();
    Ok(())
}


Review comment (suggestion):
I think we should use `.await` here, so the function looks like:

```rust
#[actix_rt::main]
async fn main() -> io::Result<()> {
...
        .run()
        .await
}
```

Code after:
    Server::build()
        .bind("echo", "127.0.0.1:8080", || {
            HttpService::build()
                .client_timeout(1000)
                .client_disconnect(1000)
                .finish(|mut req: Request| {
                    async move {
                        let mut body = BytesMut::new();
                        while let Some(item) = req.payload().next().await {
                            body.extend_from_slice(&item?);
                        }

                        info!("request body: {:?}", body);
                        Ok::<_, Error>(
                            Response::Ok()
                                .header(
                                    "x-head",
                                    HeaderValue::from_static("dummy value!"),
                                )
                                .body(body),
                        )
                    }
                })
                .tcp()
        })?
        .run()
        .await
}
Repository: actix/actix-web
File: src/request.rs (Rust)

Code before:
    ///
    /// Peer address is actual socket address, if proxy is used in front of
    /// actix http server, then peer address would be address of this proxy.
    ///
    /// To get client connection information `.connection_info()` should be used.
    #[inline]
    pub fn peer_addr(&self) -> Option<net::SocketAddr> {
        self.head().peer_addr
    }

    /// Get *ConnectionInfo* for the current request.
    ///
    /// This method panics if request's extensions container is already
    /// borrowed.
    #[inline]
    pub fn connection_info(&self) -> Ref<'_, ConnectionInfo> {
        ConnectionInfo::get(self.head(), &*self.app_config())
    }

    /// App config
    #[inline]
    pub fn app_config(&self) -> &AppConfig {
        &self.0.config
    }

    /// Get an application data object stored with `App::data` or `App::app_data` 
    /// methods during application configuration.
    ///
    /// If `App::data` was used to store object, use `Data<T>`:
    ///
    /// ```rust,ignore
    /// let opt_t = req.app_data::<Data<T>>();
    /// ```
    pub fn app_data<T: 'static>(&self) -> Option<&T> {
        if let Some(st) = self.0.app_data.get::<T>() {
            Some(&st)
        } else {
            None
        }
    }
}

impl HttpMessage for HttpRequest {
    type Stream = ();

    #[inline]
    /// Returns Request's headers.
    fn headers(&self) -> &HeaderMap {
        &self.head().headers
    }


Review comment (suggestion):
```suggestion
    /// Get an application data object stored with `App::data` or `App::app_data`
```

Code after:
    ///
    /// Peer address is actual socket address, if proxy is used in front of
    /// actix http server, then peer address would be address of this proxy.
    ///
    /// To get client connection information `.connection_info()` should be used.
    #[inline]
    pub fn peer_addr(&self) -> Option<net::SocketAddr> {
        self.head().peer_addr
    }

    /// Get *ConnectionInfo* for the current request.
    ///
    /// This method panics if request's extensions container is already
    /// borrowed.
    #[inline]
    pub fn connection_info(&self) -> Ref<'_, ConnectionInfo> {
        ConnectionInfo::get(self.head(), &*self.app_config())
    }

    /// App config
    #[inline]
    pub fn app_config(&self) -> &AppConfig {
        &self.0.config
    }

    /// Get an application data object stored with `App::data` or `App::app_data`
    /// methods during application configuration.
    ///
    /// If `App::data` was used to store object, use `Data<T>`:
    ///
    /// ```rust,ignore
    /// let opt_t = req.app_data::<Data<T>>();
    /// ```
    pub fn app_data<T: 'static>(&self) -> Option<&T> {
        if let Some(st) = self.0.app_data.get::<T>() {
            Some(&st)
        } else {
            None
        }
    }
}

impl HttpMessage for HttpRequest {
    type Stream = ();

    #[inline]
    /// Returns Request's headers.
    fn headers(&self) -> &HeaderMap {
        &self.head().headers
    }
Repository: actix/actix-web
File: actix-http/src/body.rs (Rust)

Code before:

        #[actix_rt::test]
        async fn skips_empty_chunks() {
            let body = BodyStream::new(stream::iter(
                ["1", "", "2"]
                    .iter()
                    .map(|&v| Ok(Bytes::from(v)) as Result<Bytes, ()>),
            ));
            assert_eq!(read_all(body).await.unwrap(), Bytes::from("12"));
        }
    }

    mod sized_stream {
        use super::*;

        #[actix_rt::test]
        async fn skips_empty_chunks() {
            let body = SizedStream::new(
                2,
                stream::iter(
                    ["1", "", "2"]
                        .iter()
                        .map(|&v| Ok(Bytes::from(v))),
                ),
            );
            assert_eq!(read_all(body).await.unwrap(), Bytes::from("12"));
        }
    }

    async fn read_all<B: MessageBody>(mut body: B) -> Result<Bytes, Error> {
        use futures::StreamExt as _;

        let mut bytes = BytesMut::new();
        while let Some(b) = stream::poll_fn(|cx| body.poll_next(cx)).next().await {
            bytes.put(b?);
        }
        Ok(bytes.freeze())
    }
}


Review comment (suggestion):
These tests pass with the old code that doesnt filter empty chunks.

It's clearer and more explicit to write the tests for this as test_vec, using manual polling and output checking. Eg:

```rust
assert_eq!(
    poll_fn(|cx| body.poll_next(cx)).await.unwrap().ok(),
    Some(Bytes::from("1"))
);
assert_eq!(
    poll_fn(|cx| body.poll_next(cx)).await.unwrap().ok(),
    Some(Bytes::from("2"))
);
```

Code after:
        #[actix_rt::test]
        async fn skips_empty_chunks() {
            let mut body = BodyStream::new(stream::iter(
                ["1", "", "2"]
                    .iter()
                    .map(|&v| Ok(Bytes::from(v)) as Result<Bytes, ()>),
            ));
            assert_eq!(
                poll_fn(|cx| body.poll_next(cx)).await.unwrap().ok(),
                Some(Bytes::from("1")),
            );
            assert_eq!(
                poll_fn(|cx| body.poll_next(cx)).await.unwrap().ok(),
                Some(Bytes::from("2")),
            );
        }
    }

    mod sized_stream {
        use super::*;

        #[actix_rt::test]
        async fn skips_empty_chunks() {
            let mut body = SizedStream::new(
                2,
                stream::iter(["1", "", "2"].iter().map(|&v| Ok(Bytes::from(v)))),
            );
            assert_eq!(
                poll_fn(|cx| body.poll_next(cx)).await.unwrap().ok(),
                Some(Bytes::from("1")),
            );
            assert_eq!(
                poll_fn(|cx| body.poll_next(cx)).await.unwrap().ok(),
                Some(Bytes::from("2")),
            );
        }
    }
}
Repository: actix/actix-web
File: actix-http/src/client/config.rs (Rust)

Code before:
use std::time::Duration;

// These values are taken from hyper/src/proto/h2/client.rs
const DEFAULT_H2_CONN_WINDOW: u32 = 1024 * 1024 * 2; // 2mb
const DEFAULT_H2_STREAM_WINDOW: u32 = 1024 * 1024 * 1; // 1mb

/// Connector configuration
#[derive(Clone)]
pub(crate) struct ConnectorConfig {
    pub(crate) timeout: Duration,
    pub(crate) conn_lifetime: Duration,
    pub(crate) conn_keep_alive: Duration,
    pub(crate) disconnect_timeout: Option<Duration>,
    pub(crate) limit: usize,
    pub(crate) conn_window_size: u32,
    pub(crate) stream_window_size: u32,
}

impl Default for ConnectorConfig {
    fn default() -> Self {
        Self {
            timeout: Duration::from_secs(1),
            conn_lifetime: Duration::from_secs(75),
            conn_keep_alive: Duration::from_secs(15),
            disconnect_timeout: Some(Duration::from_millis(3000)),
            limit: 100,
            conn_window_size: DEFAULT_H2_CONN_WINDOW,
            stream_window_size: DEFAULT_H2_STREAM_WINDOW,
        }
    }

Review comment (suggestion):
```suggestion
const DEFAULT_H2_STREAM_WINDOW: u32 = 1024 * 1024; // 1mb
```

Code after:
use std::time::Duration;

// These values are taken from hyper/src/proto/h2/client.rs
const DEFAULT_H2_CONN_WINDOW: u32 = 1024 * 1024 * 2; // 2mb
const DEFAULT_H2_STREAM_WINDOW: u32 = 1024 * 1024; // 1mb

/// Connector configuration
#[derive(Clone)]
pub(crate) struct ConnectorConfig {
    pub(crate) timeout: Duration,
    pub(crate) conn_lifetime: Duration,
    pub(crate) conn_keep_alive: Duration,
    pub(crate) disconnect_timeout: Option<Duration>,
    pub(crate) limit: usize,
    pub(crate) conn_window_size: u32,
    pub(crate) stream_window_size: u32,
}

impl Default for ConnectorConfig {
    fn default() -> Self {
        Self {
            timeout: Duration::from_secs(1),
            conn_lifetime: Duration::from_secs(75),
            conn_keep_alive: Duration::from_secs(15),
            disconnect_timeout: Some(Duration::from_millis(3000)),
            limit: 100,
            conn_window_size: DEFAULT_H2_CONN_WINDOW,
            stream_window_size: DEFAULT_H2_STREAM_WINDOW,
        }
    }
Repository: actix/actix-web
File: src/guard.rs (Rust)

Code before:
where
    F: Fn(&RequestHead) -> bool,
{
    fn check(&self, head: &RequestHead) -> bool {
        (self)(head)
    }
}

/// Return guard that matches if any of supplied guards.
///
/// ```rust
/// use actix_web::{web, guard, App, HttpResponse};
///
/// fn main() {
///     App::new().service(web::resource("/index.html").route(
///         web::route()
///              .guard(guard::Any(guard::Get()).or(guard::Post()))
///              .to(|| HttpResponse::MethodNotAllowed()))
///     );
/// }
/// ```
pub fn Any<F: Guard + 'static>(guard: F) -> AnyGuard {
    AnyGuard(vec![Box::new(guard)])
}

/// Matches if any of supplied guards match.
pub struct AnyGuard(Vec<Box<dyn Guard>>);

impl AnyGuard {
    /// Add guard to a list of guards to check
    pub fn or<F: Guard + 'static>(mut self, guard: F) -> Self {
        self.0.push(Box::new(guard));
        self
    }
}

impl Guard for AnyGuard {
    fn check(&self, req: &RequestHead) -> bool {
        for p in &self.0 {
            if p.check(req) {
                return true;
            }
        }
        false
    }
}

/// Return guard that matches if all of the supplied guards.
///
/// ```rust
/// use actix_web::{guard, web, App, HttpResponse};

Review comment (suggestion):
I think it's clearer:

```suggestion
/// Matches any of supplied guards.
```

Code after:
where
    F: Fn(&RequestHead) -> bool,
{
    fn check(&self, head: &RequestHead) -> bool {
        (self)(head)
    }
}

/// Return guard that matches if any of supplied guards.
///
/// ```rust
/// use actix_web::{web, guard, App, HttpResponse};
///
/// fn main() {
///     App::new().service(web::resource("/index.html").route(
///         web::route()
///              .guard(guard::Any(guard::Get()).or(guard::Post()))
///              .to(|| HttpResponse::MethodNotAllowed()))
///     );
/// }
/// ```
pub fn Any<F: Guard + 'static>(guard: F) -> AnyGuard {
    AnyGuard(vec![Box::new(guard)])
}

/// Matches any of supplied guards.
pub struct AnyGuard(Vec<Box<dyn Guard>>);

impl AnyGuard {
    /// Add guard to a list of guards to check
    pub fn or<F: Guard + 'static>(mut self, guard: F) -> Self {
        self.0.push(Box::new(guard));
        self
    }
}

impl Guard for AnyGuard {
    fn check(&self, req: &RequestHead) -> bool {
        for p in &self.0 {
            if p.check(req) {
                return true;
            }
        }
        false
    }
}

/// Return guard that matches if all of the supplied guards.
///
/// ```rust
/// use actix_web::{guard, web, App, HttpResponse};