ronantakizawa/codereview-bench huggingface dataset:
Repository: jaytaph/gosub-browser
File: src/html5_parser/parser/document.rs (Rust)

Code before:
}

#[derive(Debug)]
pub struct DocumentHandle(Rc<RefCell<Document>>);

impl Display for DocumentHandle {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        write!(f, "{}", self.0.borrow())
    }
}

impl PartialEq for DocumentHandle {
    fn eq(&self, other: &Self) -> bool {
        self.0.eq(&other.0)
    }
}

impl Clone for DocumentHandle {
    fn clone(&self) -> DocumentHandle {
        DocumentHandle(Rc::clone(&self.0))
    }
}

impl Eq for DocumentHandle {}

impl DocumentHandle {
    pub fn get(&self) -> impl Deref<Target = Document> + '_ {
        self.0.borrow()
    }

    pub fn get_mut(&mut self) -> impl DerefMut<Target = Document> + '_ {
        self.0.borrow_mut()
    }

    fn add_element(&mut self, parent_id: NodeId, name: &str) -> NodeId {
        let node = Node::new_element(self, name, HashMap::new(), HTML_NAMESPACE);
        self.get_mut().add_node(node, parent_id)
    }

    fn add_text(&mut self, parent_id: NodeId, text: &str) -> NodeId {
        let node = Node::new_text(self, text);
        self.get_mut().add_node(node, parent_id)
    }
}

#[cfg(test)]
mod tests {
    use crate::html5_parser::node::HTML_NAMESPACE;
    use crate::html5_parser::parser::{Document, Node, NodeData, NodeId};
    use std::collections::HashMap;


Review comment (suggestion):
I like keeping `Document::clone(...)`, copying `Rc::clone(&reference)` instead of `reference.clone()`, as it communicates to the developer that we're only doing a fast, shallow clone of a reference.

Code after:
}

#[derive(Debug)]
pub struct DocumentHandle(Rc<RefCell<Document>>);

impl Display for DocumentHandle {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        write!(f, "{}", self.0.borrow())
    }
}

impl PartialEq for DocumentHandle {
    fn eq(&self, other: &Self) -> bool {
        self.0.eq(&other.0)
    }
}

// NOTE: it is preferred to use Document::clone() when
// copying a DocumentHandle reference. However, for
// any structs using this handle that use #[derive(Clone)],
// this implementation is required.
impl Clone for DocumentHandle {
    fn clone(&self) -> DocumentHandle {
        DocumentHandle(Rc::clone(&self.0))
    }
}

impl Eq for DocumentHandle {}

impl DocumentHandle {
    pub fn get(&self) -> impl Deref<Target = Document> + '_ {
        self.0.borrow()
    }

    pub fn get_mut(&mut self) -> impl DerefMut<Target = Document> + '_ {
        self.0.borrow_mut()
    }

    fn add_element(&mut self, parent_id: NodeId, name: &str) -> NodeId {
        let node = Node::new_element(self, name, HashMap::new(), HTML_NAMESPACE);
        self.get_mut().add_node(node, parent_id)
    }

    fn add_text(&mut self, parent_id: NodeId, text: &str) -> NodeId {
        let node = Node::new_text(self, text);
        self.get_mut().add_node(node, parent_id)
    }
}

#[cfg(test)]
mod tests {
Repository: jaytaph/gosub-browser
File: src/css3/new_tokenizer.rs (Rust)

Code before:

        // type should be "number"
        value.push_str(&self.consume_digits());

        // todo: move them to gobal constants
        // U+0045: LATIN CAPITAL LETTER E (E)
        // U+0065: LATIN SMALL LETTER E (e)
        if self.stream.current_char().utf8() == '\u{0045}'
            || self.stream.current_char().utf8() == '\u{0065}'
        {
            value.push(self.stream.read_char().utf8());
            kind = NumberKind::Number;

            if self.stream.current_char().utf8() == '-' || self.stream.current_char().utf8() == '+'
            {
                value.push(self.stream.read_char().utf8());
            }
        }

        value.push_str(&self.consume_digits());

        Number::new(
            kind,
            value
                .parse()
                .expect("Unexpected error, should never happen"),
        )
    }

    pub fn consume_digits(&mut self) -> String {
        let mut value = String::new();

        while self.stream.current_char().utf8().is_numeric() {
            value.push(self.stream.read_char().utf8())
        }

        value
    }

    pub fn consume_chars(&mut self, mut len: usize) -> String {
        let mut value = String::new();

        while len > 0 {
            value.push(self.stream.read_char().utf8());
            len -= 1
        }

        value
    }
}


Review comment (bug):
```suggestion
                .expect("failed to parse number"),
```

Code after:

        // type should be "number"
        value.push_str(&self.consume_digits());

        // todo: move them to gobal constants
        // U+0045: LATIN CAPITAL LETTER E (E)
        // U+0065: LATIN SMALL LETTER E (e)
        if self.stream.current_char().utf8() == '\u{0045}'
            || self.stream.current_char().utf8() == '\u{0065}'
        {
            value.push(self.stream.read_char().utf8());
            kind = NumberKind::Number;

            if self.stream.current_char().utf8() == '-' || self.stream.current_char().utf8() == '+'
            {
                value.push(self.stream.read_char().utf8());
            }
        }

        value.push_str(&self.consume_digits());

        Number::new(kind, value.parse().expect("failed to parse number"))
    }

    pub fn consume_digits(&mut self) -> String {
        let mut value = String::new();

        while self.stream.current_char().utf8().is_numeric() {
            value.push(self.stream.read_char().utf8())
        }

        value
    }

    pub fn consume_chars(&mut self, mut len: usize) -> String {
        let mut value = String::new();

        while len > 0 {
            value.push(self.stream.read_char().utf8());
            len -= 1
        }

        value
    }
}

#[cfg(test)]
mod test {
    use crate::html5_parser::input_stream::Encoding;

    use super::*;
Repository: jaytaph/gosub-browser
File: src/css3/new_tokenizer.rs (Rust)

Code before:

            if self.is_any_of(vec!['"', '\'']) {
                return Token::Function(value);
            }

            return self.consume_url();
        } else if self.stream.current_char().utf8() == '(' {
            // consume '('
            self.stream.read_char();
            return Token::Function(value);
        }

        Token::Ident(value)
    }

    /// 4.3.6. [Consume a url token](https://www.w3.org/TR/css-syntax-3/#consume-a-url-token)
    ///
    /// Returns either a `<url-token>` or a `<bad-url-token>`
    pub fn consume_url(&mut self) -> Token {
        let mut url = String::new();

        self.consume_whitespace();

        loop {
            if self.stream.current_char().utf8() == ')' {
                println!("here..");
                // consume ')'
                self.stream.read_char();
                break;
            }

            if self.stream.eof() {
                // parser error
                break;
            }

            if self.is_whitespace() {
                self.consume_whitespace();
                continue;
            }

            if self.is_any_of(vec!['"', '\'', '(']) || self.is_non_printable_char() {
                // parse error
                self.consume_remnants_of_bad_url();
                return Token::BadUrl(url);
            }

            if self.is_start_of_escape() {
                url.push(self.consume_escaped_token());
                continue;
            }

Review comment (bug):
Just adding a reminder to remove this `println` later, assuming it was being used for debugging

Code after:

            if self.is_any_of(vec!['"', '\'']) {
                return Token::Function(value);
            }

            return self.consume_url();
        } else if self.stream.current_char().utf8() == '(' {
            // consume '('
            self.stream.read_char();
            return Token::Function(value);
        }

        Token::Ident(value)
    }

    /// 4.3.6. [Consume a url token](https://www.w3.org/TR/css-syntax-3/#consume-a-url-token)
    ///
    /// Returns either a `<url-token>` or a `<bad-url-token>`
    pub fn consume_url(&mut self) -> Token {
        let mut url = String::new();

        self.consume_whitespace();

        loop {
            if self.stream.current_char().utf8() == ')' {
                // consume ')'
                self.stream.read_char();
                break;
            }

            if self.stream.eof() {
                // parser error
                break;
            }

            if self.is_whitespace() {
                self.consume_whitespace();
                continue;
            }

            if self.is_any_of(vec!['"', '\'', '(']) || self.is_non_printable_char() {
                // parse error
                self.consume_remnants_of_bad_url();
                return Token::BadUrl(url);
            }

            if self.is_start_of_escape() {
                url.push(self.consume_escaped_token());
                continue;
            }
Repository: jaytaph/gosub-browser
File: src/render_tree/text.rs (Rust)

Code before:
static DEFAULT_FONT: &str = "Times New Roman";

#[derive(Debug, PartialEq)]
pub struct TextNode {
    /// Body of the text node that will be drawn
    pub value: String,
    // TODO: change this to a stack-allocated string
    pub font: String,
    pub font_size: f64,
    pub is_bold: bool,
    // TODO: color, styles, visiblity, etc
}

impl TextNode {
    /*
        NOTE: I got the default font sizes from https://stackoverflow.com/a/70720104
    */

    fn new(fs: f64) -> Self {
        Self {
            value: "".to_owned(),
            font: DEFAULT_FONT.to_owned(),
            font_size: fs,
            is_bold: false,
        }
    }

    pub fn new_heading1() -> Self {
        TextNode::new(37.)
    }

    pub fn new_heading2() -> Self {
        TextNode::new(27.5)
    }

    pub fn new_heading3() -> Self {
        TextNode::new(21.5)
    }

    pub fn new_heading4() -> Self {
        TextNode::new(18.5)
    }

    pub fn new_heading5() -> Self {

Review comment (suggestion):
I would add a `is_bold` parameter here which is used by the headings

```rust
pub fn new_heading1() -> Self {
    TextNode::new(37., true)
}
```

since right now it defaults to false and none of the headings being created are bold

Code after:
static DEFAULT_FONT: &str = "Times New Roman";

#[derive(Debug, PartialEq)]
pub struct TextNode {
    /// Body of the text node that will be drawn
    pub value: String,
    // TODO: change this to a stack-allocated string
    pub font: String,
    pub font_size: f64,
    pub is_bold: bool,
    // TODO: color, styles, visiblity, etc
}

impl TextNode {
    /*
        NOTE: I got the default font sizes from https://stackoverflow.com/a/70720104
    */

    fn new(fs: f64, bold: bool) -> Self {
        Self {
            value: "".to_owned(),
            font: DEFAULT_FONT.to_owned(),
            font_size: fs,
            is_bold: bold,
        }
    }

    pub fn new_heading1() -> Self {
        TextNode::new(37., true)
    }

    pub fn new_heading2() -> Self {
        TextNode::new(27.5, true)
    }

    pub fn new_heading3() -> Self {
        TextNode::new(21.5, true)
    }

    pub fn new_heading4() -> Self {
        TextNode::new(18.5, true)
    }

    pub fn new_heading5() -> Self {
Repository: jaytaph/gosub-browser
File: src/dns/remote.rs (Rust)

Code before:
                _ => {}
            }
        }

        if !entry.has_ipv4 && !entry.has_ipv6 {
            return Err(types::Error::DnsNoIpAddressFound);
        }

        Ok(entry)
    }

    fn name(&self) -> &'static str {
        "remote resolver"
    }
}

impl RemoteResolver {
    /// Instantiates a new local override table
    pub fn new(_opts: RemoteResolverOptions) -> RemoteResolver {
        // @todo: do something with the options
        let config = ResolverConfig::default();
        let opts = ResolverOpts::default();

        RemoteResolver {
            hickory: Resolver::new(config, opts).unwrap(),
        }
    }
}

/// Options for the remote resolver
pub struct RemoteResolverOptions {
    pub timeout: u32,
    pub retries: u32,
    pub nameservers: Vec<String>,
}

impl Default for RemoteResolverOptions {
    fn default() -> Self {
        RemoteResolverOptions {
            timeout: 5,
            retries: 3,
            nameservers: vec![],
        }
    }
}


Review comment (suggestion):
I guess, you don't need `as_str` here, because it is already a reference to a string. `&String` also counts as `&str`. But feel free to ignore.

Code after:
                }
                _ => {}
            }
        }

        if !entry.has_ipv4 && !entry.has_ipv6 {
            return Err(types::Error::DnsNoIpAddressFound);
        }

        Ok(entry)
    }

    fn name(&self) -> &'static str {
        "remote resolver"
    }
}

impl RemoteResolver {
    /// Instantiates a new local override table
    pub fn new(dns_opts: RemoteResolverOptions) -> RemoteResolver {
        // @todo: do something with the options
        let mut config = ResolverConfig::default();
        let mut opts = ResolverOpts::default();

        for nameserver in dns_opts.nameservers.iter() {
            if let Ok(ip) = IpAddr::from_str(nameserver.as_str()) {
                config.add_name_server(NameServerConfig::new(SocketAddr::new(ip, 53), Udp));
                continue;
            }
        }
        opts.use_hosts_file = dns_opts.use_hosts_file;
        opts.timeout = std::time::Duration::from_secs(dns_opts.timeout as u64);
        opts.attempts = dns_opts.retries;

        RemoteResolver {
            hickory: Resolver::new(config, opts).unwrap(),
        }
    }
}

/// Options for the remote resolver
pub struct RemoteResolverOptions {
    pub timeout: usize,
    pub retries: usize,
    pub use_hosts_file: bool,
    pub nameservers: Vec<String>,
}

impl Default for RemoteResolverOptions {
    fn default() -> Self {
        RemoteResolverOptions {
Repository: jaytaph/gosub-browser
File: .github/workflows/check_authors.sh (Shell)

Code before:
#!/bin/bash

is_in_authors() {
  TARGET=$1
  if grep -Fq "$TARGET" AUTHORS; then
    return 1
  else
    return 0
  fi
}

# Only check the first 10 committers found in the PR
COMMITTERS=$(git log $1 --pretty=format:"%an;%ae" | sort | uniq | head -n 10)

for COMMITTER in "$COMMITTERS" ; do
  # split sentence in two parts seperated by a ;
  NAME=$(echo $COMMITTER | cut -d ";" -f 1)
  EMAIL=$(echo $COMMITTER | cut -d ";" -f 2)

  if is_in_authors "$EMAIL" == 0 && is_in_authors "$NAME" == 0; then
    echo "This author is not found in the AUTHORS file"
    exit 1
  fi
done

echo "All authors are found in the AUTHORS file."
exit 0


Review comment (security):
```suggestion
is_in_authors() {
  local target=$1
  if grep -Fq "$target" AUTHORS; then
    return 1
  else
    return 0
  fi
}
```

Code after:
#!/bin/bash

is_in_authors() {
  local target=$1
  if grep -Fq "$target" AUTHORS; then
    return 1
  else
    return 0
  fi
}

# Only check the first 10 committers found in the PR
COMMITTERS=$(git log $1 --pretty=format:"%an;%ae" | sort | uniq | head -n 10)

for committer in "$COMMITTERS" ; do
  # split sentence in two parts seperated by a ;
  local name=$(echo $committer | cut -d ";" -f 1)
  local email=$(echo $committer | cut -d ";" -f 2)

  if is_in_authors "$email" == 0 && is_in_authors "$name" == 0; then
    echo "Author $name <$email> was not found in the AUTHORS file"
    exit 1
  fi
done

echo "All authors are found in the AUTHORS file."
exit 0
Repository: jaytaph/gosub-browser
File: .github/workflows/check_authors.sh (Shell)

Code before:
#!/bin/bash

is_in_authors() {
  TARGET=$1
  if grep -Fq "$TARGET" AUTHORS; then
    return 1
  else
    return 0
  fi
}

# Only check the first 10 committers found in the PR
COMMITTERS=$(git log $1 --pretty=format:"%an;%ae" | sort | uniq | head -n 10)

for COMMITTER in "$COMMITTERS" ; do
  # split sentence in two parts seperated by a ;
  NAME=$(echo $COMMITTER | cut -d ";" -f 1)
  EMAIL=$(echo $COMMITTER | cut -d ";" -f 2)

  if is_in_authors "$EMAIL" == 0 && is_in_authors "$NAME" == 0; then
    echo "This author is not found in the AUTHORS file"
    exit 1
  fi
done

echo "All authors are found in the AUTHORS file."
exit 0


Review comment (security):
```suggestion
for committer in "$COMMITTERS" ; do
  # split sentence in two parts seperated by a ;
  local name=$(echo $committer | cut -d ";" -f 1)
  local email=$(echo $committer | cut -d ";" -f 2)

  if is_in_authors "$email" == 0 && is_in_authors "$name" == 0; then
    echo "Author $name <$email> was not found in the AUTHORS file"
    exit 1
  fi
done
```

Code after:
#!/bin/bash

is_in_authors() {
  local target=$1
  if grep -Fq "$target" AUTHORS; then
    return 1
  else
    return 0
  fi
}

# Only check the first 10 committers found in the PR
COMMITTERS=$(git log $1 --pretty=format:"%an;%ae" | sort | uniq | head -n 10)

for committer in "$COMMITTERS" ; do
  # split sentence in two parts seperated by a ;
  local name=$(echo $committer | cut -d ";" -f 1)
  local email=$(echo $committer | cut -d ";" -f 2)

  if is_in_authors "$email" == 0 && is_in_authors "$name" == 0; then
    echo "Author $name <$email> was not found in the AUTHORS file"
    exit 1
  fi
done

echo "All authors are found in the AUTHORS file."
exit 0
Repository: jaytaph/gosub-browser
File: src/net.rs (Rust)

Code before:
//! Networking functionality
//!
//! This module contains all the networking functionality for the browser. THis is normally the
//! lowlevel implementation of the browser. The networking module is responsible for making HTTP
//! requests, parsing the response and returning the result to the caller.
//!
//! It also contains additional networking components like the DNS resolver.
#[allow(dead_code)]
pub mod dns;
pub mod errors;
pub mod http;


Review comment (suggestion):
```suggestion
//! This module contains all the networking functionality for the browser. This is normally the
```

Code after:
//! Networking functionality
//!
//! This module contains all the networking functionality for the browser. This is normally the
//! lowlevel implementation of the browser. The networking module is responsible for making HTTP
//! requests, parsing the response and returning the result to the caller.
//!
//! It also contains additional networking components like the DNS resolver.
#[allow(dead_code)]
pub mod dns;
pub mod errors;
pub mod http;
Repository: jaytaph/gosub-browser
File: src/bin/gosub-parser.rs (Rust)

Code before:
use anyhow::{bail, Result};
use gosub_html5::parser::document::{Document, DocumentBuilder};
use gosub_html5::parser::Html5Parser;
use gosub_shared::bytes::{CharIterator, Confidence, Encoding};
use std::fs;

fn main() -> Result<()> {
    let url = std::env::args()
        .nth(1)
        .unwrap_or_else(|| bail!("Usage: gosub-parser <url>"));

    let html = if url.starts_with("http://") || url.starts_with("https://") {
        // Fetch the html from the url
        let response = ureq::get(&url).call()?;
        if response.status() != 200 {
            bail!(&format!(
                "Could not get url. Status code {}",
                response.status()
            ));
        }
        response.into_string()?
    } else {
        // Get html from the file
        fs::read_to_string(&url)?
    };

    let mut chars = CharIterator::new();
    chars.read_from_str(&html, Some(Encoding::UTF8));
    chars.set_confidence(Confidence::Certain);

    // If the encoding confidence is not Confidence::Certain, we should detect the encoding.
    if !chars.is_certain_encoding() {
        chars.detect_encoding();
    }

    let document = DocumentBuilder::new_document();
    let parse_errors = Html5Parser::parse_document(&mut chars, Document::clone(&document), None)?;

    println!("Generated tree: \n\n {document}");

    for e in parse_errors {
        println!("Parse Error: {}", e.message);
    }


Review comment (suggestion):
The bail macro supports formatting arguments so you don't have to explicitly use the format macro.
(See https://docs.rs/anyhow/latest/anyhow/macro.bail.html)
```suggestion
            bail!("Could not get url. Status code {}", response.status());
```

Code after:
use anyhow::bail;
use gosub_html5::parser::document::{Document, DocumentBuilder};
use gosub_html5::parser::Html5Parser;
use gosub_shared::bytes::{CharIterator, Confidence, Encoding};
use gosub_shared::types::Result;
use std::fs;

fn main() -> Result<()> {
    let matches = clap::Command::new("Gosub parser")
        .version("0.1.0")
        .arg(
            clap::Arg::new("url")
                .help("The url or file to parse")
                .required(true)
                .index(1),
        )
        .get_matches();

    let url = matches
        .get_one::<String>("url")
        .ok_or("Missing url")
        .unwrap()
        .to_string();

    let html = if url.starts_with("http://") || url.starts_with("https://") {
        // Fetch the html from the url
        let response = ureq::get(&url).call()?;
        if response.status() != 200 {
            bail!("Could not get url. Status code {}", response.status());
        }
        response.into_string()?
    } else {
        // Get html from the file
        fs::read_to_string(&url)?
    };

    let mut chars = CharIterator::new();
    chars.read_from_str(&html, Some(Encoding::UTF8));
    chars.set_confidence(Confidence::Certain);

    // If the encoding confidence is not Confidence::Certain, we should detect the encoding.
    if !chars.is_certain_encoding() {
        chars.detect_encoding();
    }