Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add rust edition feature #12

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

MahadMuhammad
Copy link
Contributor

src/known-directives.rs Outdated Show resolved Hide resolved
src/known-directives.rs Outdated Show resolved Hide resolved
Add `regex` and exported it for other files

Signed-off-by: Muhammad Mahad <[email protected]>
Signed-off-by: Muhammad Mahad <[email protected]>
Signed-off-by: Muhammad Mahad <[email protected]>
Signed-off-by: Muhammad Mahad <[email protected]>
Signed-off-by: Muhammad Mahad <[email protected]>
@MahadMuhammad MahadMuhammad force-pushed the feat/add-additional-options branch 2 times, most recently from 2a82d33 to 15d5640 Compare August 21, 2024 18:34
Signed-off-by: Muhammad Mahad <[email protected]>
If the test source file doesn't contain any headers, or
that are not added till now then the header value
will be None and becomes panic

Signed-off-by: Muhammad Mahad <[email protected]>
Copy link
Member

@P-E-P P-E-P left a comment

Choose a reason for hiding this comment

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

A lot of great things in there, most of my comments are nitpicks because you did really great 😆

Comment on lines +75 to +76
/// clap reports most development errors as `debug_assert!`s
/// See this for more details, [here](https://docs.rs/clap/4.5.15/clap/_derive/_tutorial/chapter_4/index.html)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure a doc comment is required ? A regular comment might be enough

std::{fmt, str::FromStr},
};

// https://docs.rs/once_cell/1.19.0/once_cell/#lazily-compiled-regex
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// https://docs.rs/once_cell/1.19.0/once_cell/#lazily-compiled-regex
/// Macro used to lazily create a new regex the first time it is invoked.
///
/// # Arguments
///
/// * `re` - The regex literal string used to build the automaton
///
/// # Example
///
/// ```rust
/// assert!(regex!(r"\w").is_match(" "));
/// ```
///
/// Taken from here https://docs.rs/once_cell/1.19.0/once_cell/#lazily-compiled-regex

Macros can be tricky to understand, I'd like this to be more thoroughly documented. A doctest example even provides a great insight (the example should compile & run when you launch cargo test).

@@ -60,11 +72,14 @@ pub struct Error {
/// What kind of message we expect (e.g., warning, error, suggestion).
/// `None` if not specified or unknown message kind.
pub kind: Option<RustcErrorKind>,
///Note: if we are loading this from rustc source file, this might be incomplete
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
///Note: if we are loading this from rustc source file, this might be incomplete
/// Note: if we are loading this from rustc source file, this might be incomplete

Comment on lines +81 to +82
/// Formats the `Error` for display according to `DejaGnu` format
/// See `DejaGnu` documentation [here](https://gcc.gnu.org/onlinedocs/gccint/testsuites/directives-used-within-dejagnu-tests/syntax-and-descriptions-of-test-directives.html)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Formats the `Error` for display according to `DejaGnu` format
/// See `DejaGnu` documentation [here](https://gcc.gnu.org/onlinedocs/gccint/testsuites/directives-used-within-dejagnu-tests/syntax-and-descriptions-of-test-directives.html)
/// Formats the `Error` for display according to `DejaGnu` format
/// See [`DejaGnu` documentation](https://gcc.gnu.org/onlinedocs/gccint/testsuites/directives-used-within-dejagnu-tests/syntax-and-descriptions-of-test-directives.html)

pub msg: String,
pub error_code: Option<String>,
}

impl fmt::Display for Error {
/// Formats the `Error` for display according to `DejaGnu` format
/// See `DejaGnu` documentation [here](https://gcc.gnu.org/onlinedocs/gccint/testsuites/directives-used-within-dejagnu-tests/syntax-and-descriptions-of-test-directives.html)
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
use RustcErrorKind::*;

Copy link
Member

Choose a reason for hiding this comment

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

let error_type = match &self.kind {
            Some(Help) => "help",
            Some(Note) => "dg-note",
            Some(Suggestion) => "suggestion",
            Some(Warning) => "dg-warning",
            Some(Error) | None => "dg-error",
};

Some(HeaderLine {
line_number: line_number + 1, // 1 based-indexed instead of zero based
_directive: "edition",
dejagnu_header: to_dejagnu_edition(edition.unwrap().as_str()),
Copy link
Member

Choose a reason for hiding this comment

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

Why can we unwrap edition safely without panicking here ?

Comment on lines +65 to +67
panic!(
"malformed condition directive: expected `{comment}[foo]`, found `{original_line}`"
)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not thrilled about this panic here

@@ -0,0 +1,233 @@
// Copied from https://github.com/rust-lang/rust/blob/master/src/tools/compiletest/src/command-list.rs
Copy link
Member

Choose a reason for hiding this comment

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

We want this source to appear in the documentation

@@ -1,8 +1,13 @@
use anyhow::{Context, Result};
use clap::Parser;
//! The main entry point of the program.
Copy link
Member

Choose a reason for hiding this comment

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

No need to comment main.rs like this :) File is called main.rs and has a main function it is fairly obvious is the main entry point.

// TODO: This is not the efficient way to find respective line number
for error in errors.iter() {
// Checking the original line number
if (error.line_num as i32 - error.relative_line_num) != line_num as i32 {
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer if you could use either the From trait or the TryFrom trait (or their Into counterpart) instead of a cast because I don't like silent errors that morph into bugs.

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.

Add parsing additional options
3 participants