Feedback #1

Muscraft · 2025-03-29T15:53:44Z

No description provided.

src/snippet.rs

coveralls · 2025-03-29T15:59:29Z

Pull Request Test Coverage Report for Build 14500512388

Details

1246 of 1415 (88.06%) changed or added relevant lines in 7 files are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage increased (+1.4%) to 87.161%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/lib.rs	1	2	50.0%
src/level.rs	17	24	70.83%
src/snippet.rs	94	118	79.66%
src/renderer/source_map.rs	248	287	86.41%
src/renderer/mod.rs	858	956	89.75%

Files with Coverage Reduction	New Missed Lines	%
src/snippet.rs	2	81.43%

Totals
Change from base Build 14148835326:	1.4%
Covered Lines:	1351
Relevant Lines:	1550

💛 - Coveralls

epage · 2025-03-31T15:49:25Z

examples/footer.svg

note: doesn't get colored?

epage · 2025-03-31T15:51:44Z

examples/format.svg

Changes seem to be

We now bold the : in error:

We will no longer color the (whitespace) between the column and |

If its not too much trouble, could this be hacked in to the old renderer in a commit before so the test changes are easier to see?

epage · 2025-03-31T15:52:22Z

examples/multislice.svg

We added a blank line between the first snippet and :::

epage · 2025-03-31T15:52:54Z

examples/expected_type.svg

We are now folding lines between annotations

epage · 2025-03-31T15:53:52Z

src/renderer/mod.rs

 use margin::Margin;
-use std::fmt::Display;
+use rustc_hash::{FxHashMap, FxHasher};


should we consider making this an optional feature or removing it?

epage · 2025-03-31T15:54:44Z

src/snippet.rs

 /// Primary structure provided for formatting
 ///
 /// See [`Level::title`] to create a [`Message`]
-#[derive(Debug)]
+#[derive(Clone, Debug)]


Could you split API changes like this into a commit before hand?

epage · 2025-03-31T15:57:51Z

examples/multislice.svg

refactor: Right align line numbers

That is not a refactor if its user visible

epage · 2025-03-31T15:58:24Z

benches/bench.rs

refactor: Move to new API

its not a refactor if its user visible.

This is also a breaking change and should be called out

epage · 2025-03-31T15:59:35Z

benches/bench.rs

-                    .label("expected enum `std::option::Option`"),
-            ),
+    let message = Level::Error.message("mismatched types").id("E0308").group(
+        Group::new().element(


Could you explain the new API in the commit or PR?

For example, why was a Group container chosen over group accepting a Vec or IntoIterator

epage · 2025-03-31T16:02:50Z

src/snippet.rs

+fn newline_count(body: &str) -> usize {
+    #[cfg(feature = "simd")]
+    {
+        memchr::memchr_iter(b'\n', body.as_bytes())
+            .count()
+            .saturating_sub(1)
+    }
+    #[cfg(not(feature = "simd"))]
+    {
+        body.lines().count().saturating_sub(1)
+    }
+}
+


Generally, implementation details should be at the end.

epage · 2025-03-31T16:05:09Z

src/snippet.rs

@@ -38,104 +35,178 @@ impl<'a> Message<'a> {
        self
    }

-    pub fn snippet(mut self, slice: Snippet<'a>) -> Self {
-        self.snippets.push(slice);
+    pub fn group(mut self, group: Group<'a>) -> Self {


One of the places where we deal with groups, we'll need to explain the difference between a single group vs many groups. Unsure whether a suggested policy belongs in this package or somewhere else but at least Cargo and Rustc will need to be consistent

epage · 2025-03-31T16:06:13Z

src/snippet.rs

-                    .map(|a| a.range.start)
-                    .min()
-                    .unwrap_or(0);
+    pub fn needs_col_separator(mut self, needs_col_separator: bool) -> Self {


What role does this serve?

epage · 2025-03-31T16:07:51Z

src/snippet.rs

        self
    }

-    pub fn annotations(mut self, annotation: impl IntoIterator<Item = Annotation<'a>>) -> Self {
-        self.annotations.extend(annotation);
+    pub fn col_separator(mut self, col_separator: bool) -> Self {


What is this?

epage · 2025-03-31T16:08:27Z

src/snippet.rs

+    Primary, // in rustc terms, "primary"; color to message's level
+    Context, // in rustc terms, "secondary"; fixed color
+             // Level(Level),  // from old API, dropped


Please use doc comments

epage · 2025-03-31T16:09:35Z

src/snippet.rs

+    pub fn primary(mut self, primary: bool) -> Self {
+        self.primary = primary;
+        self
+    }
+
+    pub fn standalone(mut self, standalone: bool) -> Self {
+        self.standalone = standalone;
+        self
+    }


What are these?

epage · 2025-03-31T16:10:37Z

src/snippet.rs

+    pub fn line(mut self, line: usize) -> Self {
+        self.line = Some(line);
+        self
+    }
+
+    pub fn char_column(mut self, char_column: usize) -> Self {
+        self.char_column = Some(char_column);
+        self
+    }


These are new. How do they relate to the rest of the API?

Should we make new parts of the API an extra commit on top of the existing API for easier comparison and discussion?

epage · 2025-03-31T16:11:48Z

src/snippet.rs

+impl<'a> From<Snippet<'a, Annotation<'a>>> for Element<'a> {
+    fn from(value: Snippet<'a, Annotation<'a>>) -> Self {
+        Element::Cause(value)
+    }
+}


I'd put all of these Froms next to Element as this is about defining how to interact with an Element and not each of the other types

epage · 2025-03-31T16:14:03Z

src/snippet.rs

+    }
+
+    pub(crate) fn is_deletion(&self, sm: &SourceMap<'_>) -> bool {
+        self.replacement.trim().is_empty() && self.replaces_meaningful_content(sm)


Why trim and only on this one?

epage · 2025-03-31T16:16:52Z

tests/formatter.rs

Could we add these tests in a prior commit, either without the patches or using annotations instead of patches?

epage · 2025-03-31T16:18:10Z

src/renderer/mod.rs

+    pub const fn theme(mut self, output_theme: OutputTheme) -> Self {
+        self.theme = output_theme;
+        self
+    }
+


I feel like if I was looking for how to enable unicode support, I wouldn't be looking at theming.

epage · 2025-03-31T16:18:56Z

tests/formatter.rs

@@ -1690,3 +1691,424 @@ help: consider removing the `?Sized` bound to make the type parameter `Sized`
    let renderer = Renderer::plain();
    assert_data_eq!(renderer.render(input_new), expected);
 }
+
+#[test]
+fn e0271() {


what are these test names?

Can we add these tests without unicode support and then show how unicode support changes them?

epage · 2025-03-31T16:19:25Z

tests/formatter.rs

chore: Align more with rustc's output

"chore"s don't generally have user-visible impact. Isn't this a fix?

And in what way?

epage · 2025-03-31T16:20:25Z

Cargo.toml

refactor: Better match rustc's internal renderer

Since this has end-user impact, this isn't just about better matching the internals but the result and that should be reflected in the commit type

Ideally, the commit description would cover expected changes, including regressions

epage · 2025-04-16T16:45:36Z

src/renderer/mod.rs

fix: Use display and char pos in draw_line

That says what its doing; not what its fixing

github-advanced-security bot found potential problems Mar 29, 2025

View reviewed changes

Muscraft force-pushed the feedback branch 3 times, most recently from 6240e15 to ed8dfa6 Compare March 29, 2025 20:10

Muscraft marked this pull request as draft March 29, 2025 20:14