Move to a `Renderer` when for displaying a `Snippet` #67

Muscraft · 2023-12-02T22:28:36Z

This PR moves to using Renderer as the external type for handling displaying a Snippet.

This has a few advantages:

Keeps what is doing the actual formatting as an internal implementation detail
- In the future, different types of renderers could be used and exposed as an option to "pick"
Moves the formatting options to be part of the renderer, not an individual Snippet
Makes a clearer distinction between what is a Snippet and what is going to be "rendering" it

This PR also:

moves to anstyle for color and removes the color feature
- anstyle is lightweight and writes nothing if the color is Style::new
- anstyle is what cargo uses and it would be good if when we use this crate we only need one color library.
moves many things to be internal details

Note: this is a step in the direction of the proposals for a more modern API (#12 and #13). I have been taking inspiration from both PRs to get a clear vision of what the future should look like.

To review this PR, look at it commit by commit

epage · 2023-12-03T00:28:32Z

Note that the biggest reason to use anstyle is that it is designed primarily to exist in public APIs for interoperability. Use for styling is secondary and there exist adapters to styling APIs

epage · 2023-12-03T00:29:47Z

Note that the biggest reason to use anstyle is that it is designed primarily to exist in public APIs for interoperability. Use for styling is secondary and there exist adapters to styling APIs.

zbraniecki

lgtm!

src/renderer/margin.rs

zbraniecki · 2023-12-04T22:16:53Z

src/renderer/mod.rs

+    stylesheet: StyleSheet,
+}
+
+impl Renderer {


praise: I like that abstraction. Great addition!

zbraniecki · 2023-12-04T22:18:06Z

src/renderer/mod.rs

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


question: Why do you need those setters, why not make the properties public?

I did it because it is easier (and cleaner) to edit one specific field than making the fields public. The other thing is I believe it makes adding a field not a breaking change, but I do not know for certain.

I don't understand how is it cleaner. Comparing:

let mut renderer = Renderer::plain(); renderer.anonymized_line_numbers(true);

vs

let mut renderer = Renderer::plain(); renderer.anonymized_line_numbers = true;

Can you explain further?

The other thing is I believe it makes adding a field not a breaking change, but I do not know for certain.

That can be achieved via https://doc.rust-lang.org/reference/attributes/type_system.html#the-non_exhaustive-attribute

That's true I forgot about that. I will switch things over

I was looking at making them public, and I realized what I meant by cleaner.

let renderer = Renderer::plain() .anonymized_line_numbers(true) .margin(margin);

vs

let mut renderer = Renderer::plain(); renderer.anonymized_line_numbers = true; renderer.margin = margin;

It makes it so that renderer is not mutable and things can be chained together (omitting renderer each time).

It follows the builder pattern which I am a fan for smaller structs, for larger structs I do think making fields public is a good idea.

If you would still like me to change it to be public fields I can but it will take me a moment to refactor.

It makes it so that renderer is not mutable and things can be chained together (omitting renderer each time).

That's not correct. The renderer still has to be mutable for the setters to work (you're consuming them btw. mut self).

The builder pattern is useful when your setters actually perform computations, invariant validation etc. In this case, your builder is the struct itself, so exposing it as such allows customers to use any Rust patterns to construct it.

By hiding it you're relying on custom uninspectable API to achieve the same.

It's a glorified version of:

struct Point { x: f64, y: f64 } impl Point { fn new(x: f64, y: f64) -> Self { Self { x, y } } fn set_x(&mut self, x: f64) -> &mut Self { self.x = x; self } fn set_y(&mut self, y: f64) -> &mut Self { self.y = y; self } }

vs just:

struct Point { pub x: f64, pub y: f64, }

and let customers create it and operate on it any way they want. You can add helper impl methods (like you do with plain), but taking away ability to operate on the fields without any reason seems like a way that limits your downstream consumers from being able to interact with the struct in ways that you don't necessarily have to be able to even predict.

In other words, if at the moment of consumption, you don't care how this struct has been constructred (maybe someone created their own methods to build the Renderer, or merge two), and you don't perform any validation, or variant locking, then just let it be an open struct and let people use all of Rust expressiveness to operate on it.

Saying that - this is my opinion, not a strong requirement - I will not try to block your PR on that, so if my arguments are not convincing to you, continue with this API and I'll r+ it

You can: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=6ddedead395636105b9830d26ab0e663

Thats within the same visibility. I don't think you can if you don't have visibility.

One benefit I've seen to builder is the ability to evolve an API, particularly for evolving styles. You'll also see builder used in a lot of the ecosystem when no validation is done.

Ah, good call. Yeah, ability to use .. would be nice.

I'm generally supportive of builder patterns when there's logic involved. I find it inelegant when we have a plain struct and we populate it with dummy getters/setters.

But as I stated, this is my preference, and I'm not intending to push for it any further. I support the direction the author of the PR decides to take.

I can see the merits of both and could go either way on this. Even though it is a breaking change, we could always change in the future. I'd say we should go with the builder (for now).

src/display_list/mod.rs

src/renderer/mod.rs

src/display_list/mod.rs

src/renderer/mod.rs

zbraniecki

lgtm!

src/lib.rs

epage · 2023-12-05T01:58:30Z

src/renderer/margin.rs

+impl Margin {
+    pub fn new(
+        whitespace_left: usize,
+        span_left: usize,
+        span_right: usize,
+        label_right: usize,
+        column_width: usize,
+        max_line_len: usize,
+    ) -> Self {


In the future, we might want to consider if there are improvements to this API as well

src/renderer/mod.rs

BREAKING CHANGE: This removes `opt` from `Snippet`

BREAKING CHANGE: This removes the `formatter` and `stylesheets` modules

BREAKING CHANGE: This removes `color` feature

BREAKING CHANGE: This moves `snippet::*` and `Renderer` to root

src/lib.rs

epage

nit: refactors can't have breaking changes, so I'd change the commit types away from it where user-visible changes are made

feat: Add Renderer

9076cbf

zbraniecki reviewed Dec 4, 2023

View reviewed changes

Muscraft force-pushed the refactor branch from 5c6171b to 7e60193 Compare December 4, 2023 23:31

github-advanced-security bot found potential problems Dec 4, 2023

View reviewed changes

src/display_list/mod.rs Fixed Show fixed Hide fixed

src/renderer/mod.rs Fixed Show fixed Hide fixed

src/display_list/mod.rs Fixed Show fixed Hide fixed

src/renderer/mod.rs Fixed Show fixed Hide fixed

Muscraft force-pushed the refactor branch from 7e60193 to fff4eb9 Compare December 4, 2023 23:49

Muscraft marked this pull request as ready for review December 4, 2023 23:51

zbraniecki approved these changes Dec 5, 2023

View reviewed changes

epage reviewed Dec 5, 2023

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

epage reviewed Dec 5, 2023

View reviewed changes

src/renderer/mod.rs Outdated Show resolved Hide resolved

epage reviewed Dec 5, 2023

View reviewed changes

src/renderer/mod.rs Outdated Show resolved Hide resolved

epage reviewed Dec 5, 2023

View reviewed changes

src/renderer/mod.rs Outdated Show resolved Hide resolved

epage reviewed Dec 5, 2023

View reviewed changes

src/renderer/mod.rs Outdated Show resolved Hide resolved

epage reviewed Dec 5, 2023

View reviewed changes

src/renderer/mod.rs Outdated Show resolved Hide resolved

epage reviewed Dec 5, 2023

View reviewed changes

src/renderer/mod.rs Outdated Show resolved Hide resolved

epage reviewed Dec 5, 2023

View reviewed changes

src/renderer/mod.rs Outdated Show resolved Hide resolved

Muscraft force-pushed the refactor branch from fff4eb9 to b016a59 Compare December 5, 2023 18:40

Muscraft added 10 commits December 5, 2023 11:53

fix!: Move format options to Renderer

d0c65b2

BREAKING CHANGE: This removes `opt` from `Snippet`

fix!: Move to a new StyleSheet

4affdfb

BREAKING CHANGE: This removes the `formatter` and `stylesheets` modules

fix!: Switch to anstyle for color

dfd4e87

BREAKING CHANGE: This removes `color` feature

fix!: Move Margin to renderer

79f657e

fix!: Make display_list private

da45f48

refactor(display_list): Remove From impls

d45fbd4

refactor(display_list): Take a reference to StyleSheet

a9bf385

refactor(renderer): Make functions const

748b792

chore(renderer): Add doc comments

b0848f7

fix!: Move around items in the public api

a1007dd

BREAKING CHANGE: This moves `snippet::*` and `Renderer` to root

epage reviewed Dec 5, 2023

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

epage approved these changes Dec 5, 2023

View reviewed changes

refactor: Move display_list under renderer

71b1eb5

Muscraft force-pushed the refactor branch from b016a59 to 71b1eb5 Compare December 5, 2023 18:58

Muscraft merged commit 131a7af into rust-lang:master Dec 5, 2023
15 checks passed

Muscraft deleted the refactor branch December 5, 2023 19:03

epage mentioned this pull request Dec 5, 2023

Decouple FormatOptions from Snippet #40

Closed

Muscraft mentioned this pull request Dec 12, 2023

chore: Update README.md example to use Renderer #70

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move to a `Renderer` when for displaying a `Snippet` #67

Move to a `Renderer` when for displaying a `Snippet` #67

Muscraft commented Dec 2, 2023

epage commented Dec 3, 2023

epage commented Dec 3, 2023

zbraniecki left a comment

zbraniecki Dec 4, 2023

zbraniecki Dec 4, 2023

Muscraft Dec 4, 2023

zbraniecki Dec 4, 2023

Muscraft Dec 4, 2023

Muscraft Dec 4, 2023

zbraniecki Dec 5, 2023

zbraniecki Dec 5, 2023

epage Dec 5, 2023

zbraniecki Dec 5, 2023

Muscraft Dec 5, 2023

zbraniecki left a comment

epage Dec 5, 2023

epage left a comment

Move to a Renderer when for displaying a Snippet #67

Move to a Renderer when for displaying a Snippet #67

Conversation

Muscraft commented Dec 2, 2023

epage commented Dec 3, 2023

epage commented Dec 3, 2023

zbraniecki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbraniecki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epage left a comment

Choose a reason for hiding this comment

Move to a `Renderer` when for displaying a `Snippet` #67

Move to a `Renderer` when for displaying a `Snippet` #67