Clippy #43

vic1707 · 2025-02-26T08:50:41Z

Fixes #42 (?)

Had a bit of fun checking lints and all assuming you'd want to at least see what it looks like ^^
As you said enabling clippy::pedantic didn't trigger much (only 2 lints about numbers casts).

Feel free to checkout the branch and see what lints should be enabled (and thus fixed) or if any actual changes bother you so we can revert and bypass the lint.

Todo:

: re-organize the bypass lints into categories
: see what I can fix without changing existing behaviours

riley-williams · 2025-02-26T17:28:55Z

Cargo.toml

@@ -14,7 +14,7 @@ exclude = [".github"]
 maintenance = { status = "experimental" }

 [features]
-default = ["embedded-graphics"]
+default = ["embedded-graphics", "crossterm"]


What's the best practice / reasoning picking default features? Crossterm support feels (to me) more like a toy at this point, given there are many better choices for std TUIs in Rust. I imagine nearly everyone who will use this will be targeting no_std / embedded and thought it would reduce confusion by having the default features all be no_std.

As an aside, compile times today seem pretty quick, but I also wonder if users will prefer to opt-out of compiling all of character rendering in the future. I still go back and forth on whether this will (probably by accident) turn into a viable option for TUI, and how much effort to put into supporting it. It's certainly useful to have for testing.

This shouldn't have been committed, that's my-bad.
My editor doesn't lint or treat the code with the feature disabled.
I didn't look for an actual solution to this yet, to my knowledge a .vscode/settings.json and equivalents for other editors does the trick. I'll look into it to avoid committing that by mistake again 😓.

As for the feature themselves I'm not an expert, I'd maybe say no features by default (a compile error can be triggered if no feature are enabled if you want).
As someone probably wants one of the 2 strategies, rarely both.
Setting default to embedded-graphics means that users will also want to use default-features = false in their Cargo.toml when wanting only crossterm.

Ah no worries, I've accidentally done the same thing. I think it probably makes sense then for me to leave it as-is.

riley-williams · 2025-02-26T22:52:24Z

tests/spacer.rs

 use buoyant::font::CharacterBufferFont;
-use buoyant::layout::{Layout, LayoutDirection};
+use buoyant::layout::{Layout as _, LayoutDirection};


I like this lint to enforce as _

riley-williams · 2025-02-26T22:53:43Z

tests/padding.rs

@@ -1,18 +1,19 @@
-use std::iter::zip;
+#![cfg(test)]


What effect does this have?

clippy is fine without this line, but rust-analyzer isn't.
despite this file being in the tests folder it's sometimes not recognized as such and thus triggers an error.
I think this issue rust-lang/rust-analyzer#7225 refers to it.
In personal projects I usually put a lib.rs in the root of tests and list modules there, it seemed to fix the issue (see: https://github.com/vic1707/nnn/blob/main/tests/lib.rs)

I guess I still don't really understand what the issue is after peeking at that issue, maybe because I just have yet to see it in my personal tools/workflow. Is this an actual problem you see now or is it preemptive?

riley-williams · 2025-02-26T23:03:32Z

tests/animation.rs

@@ -517,6 +518,7 @@ fn toggle_stack(is_on: bool) -> impl Renderable<char, Renderables: CharacterRend
 }

 #[test]
+#[expect(clippy::cognitive_complexity)]


Clippy is probably right, these animation tests are very difficult to read. Fixing it is probably a good opportunity to play with wrappers that manage the view/layout/tree and cover up some of the boilerplate. No risk of needing breaking API to change later.

riley-williams · 2025-02-26T23:06:37Z

src/view/text/character.rs

@@ -1,3 +1,4 @@
+extern crate alloc;


Assuming the changes here will all go away when you update with main, but I agree with preferring alloc to std

In this case I'd suggest doing an alloc feature for the same reason the std feature exists

riley-williams · 2025-02-26T23:52:11Z

src/render/circle.rs

            let x = i16::interpolate(source.origin.x, target.origin.x, domain.factor);
            let y = i16::interpolate(source.origin.y, target.origin.y, domain.factor);
            let diameter = u16::interpolate(source.diameter, target.diameter, domain.factor);
-            Circle {
+            Self {


riley-williams · 2025-02-26T23:55:03Z

src/render/animate.rs

-            let subdomain;
-            if end_time == Duration::from_secs(0) || domain.app_time >= end_time {
-                // animation has already completed or there was zero duration
+            let subdomain = if end_time == Duration::from_secs(0) || domain.app_time >= end_time {


I lean more towards this being situational discretion than something that ought to be linted on. This is actually a great example of where the lint doesn't help lol

fair enough, clippy is opinionated and this changes most definitely comes from nursery. I'll disable it globally 👍

riley-williams · 2025-02-26T23:56:48Z

src/primitives/interpolate.rs

+#[cfg(test)]
+#[cfg(feature = "embedded-graphics")]


riley-williams · 2025-02-27T00:16:25Z

.cargo/config.toml

@@ -0,0 +1,48 @@
+[build]


Is it better to configure this with rustflags or in the Cargo.toml? I've used the Cargo.toml option to warn on pedantic as well as disabling a couple temporarily.

I tried enabling the nursery lints in another project and ended up turning it off because a lot ended up just being annoying. I assume a lot of these that have been manually allowed are probably nursery too?

I don't know if there is an accepted/idiomatic answer.
IMO Cargo.toml is included in the library publishing process and users read it, I feel like the crate's clippy config isn't of users concern (as in "users don't care", not "should be hidden") so we shouldn't bother them with it 🤔

Yep most of the disabled lints here are from nursery (each commit deals with a specific group of lints).
it's a pain, some lints contradict others some feel useless (let me do maths already 😡😂) but I don't know, I think I grew into liking them.
I view them as "nice to have" or at least "don't be afraid to disable some", it makes them more bearable

riley-williams · 2025-02-27T00:22:00Z

tests/match_view.rs

@@ -139,17 +141,17 @@ fn test_match_view_two_variants_invalid_layout() {
    let font = CharacterBufferFont;
    let make_view = |state| {
        match_view!(state => {
-            0 => Text::str("zero\n!!!", &font),
+            0i32 => Text::str("zero\n!!!", &font),


Undecided on this one

yeah, it looks weird, a solution would be to inject the #[allow()] attribute inside the macro

riley-williams · 2025-02-28T23:29:51Z

I think we'll stick to defining these in the Cargo.toml, mainly because this has advantages for workspaces which I imagine this will eventually become.

It makes importing them into the packages trivial, whereas I believe the rust flags must be duplicated for every package.

# package/Cargo.toml
[lints]
workspace = true

I think what I want for the lint selection is actually the inverse. The default lints really go a long way and like you said, they're quite powerful. Rather than dealing with random issues that pop up in unstable nursery and restriction lints, I think I'd rather cherry pick what makes the most sense for this project today.

I went through, and I think this covers the issues you've fixed. I picked out some others that I think make sense, but probably require more involved changes.

[lints.clippy]
pedantic = { level = "deny", priority = -1 }

# You fixed?
needless_pass_by_ref_mut = "deny"
use_self = "deny"
derive_partial_eq_without_eq = "deny"
too_long_first_doc_paragraph = "deny"
type_repetition_in_bounds = "deny"
option_if_let_else = "deny"
useless_let_if_seq = "deny"

# TODO: Disabled for now, desired fix
cast_possible_wrap = "allow"
cast_possible_truncation = "allow"
exhaustive_structs = "allow"
exhaustive_enums = "allow"
missing_panics_doc = "allow"
field_scoped_visibility_modifiers = "allow"
ref_patterns = "allow"
cast_sign_loss = "allow"
string_slice = "allow"
shadow_unrelated = "allow"
renamed_function_params = "allow"
float_arithmetic = "allow"
indexing_slicing = "allow"
as_conversions = "allow"
absolute_paths = "allow"                    # configure as makes sense

riley-williams · 2025-03-14T22:57:16Z

Thanks for all the effort answering my questions in your PR! I (finally) ended up merging the results of this separately in #60 to avoid the conflicts

vic1707 · 2025-03-14T23:16:09Z

Hi, I'm terribly sorry you ended up doing it yourself 😞
I just realized how long it's been since I opened my project and buoyant itself 😥
Life is a bit of a mess right now, hopefully I'll come back to your wonderful project asap !

riley-williams · 2025-03-14T23:37:22Z

Life gets busy, no worries. Running clippy --fix was the easiest part after figuring out what lints to keep here.
On the bright side, the longer you wait, the better the documentation gets!
Hope to see you back and that everything settles down soon.

vic1707 added 8 commits February 25, 2025 18:12

minimal clippy fix

0245c0f

enable clippy::nursery + clippy --fix

c238b7c

clippy::nursery manual fixes

a7d347b

move clippy config

d084579

enable clippy pedantic

cd9df2e

enable clippy::restriction minus conflicts + clippy --fix

75cd103

some clippy::restriction fixes

3d0a8b1

move to clippy::all

321d43b

riley-williams reviewed Feb 27, 2025

View reviewed changes

riley-williams closed this Mar 14, 2025

vic1707 deleted the clippy-2 branch March 24, 2025 10:59

		#[cfg(test)]
		#[cfg(feature = "embedded-graphics")]

Uh oh!

Clippy #43

Clippy #43

Uh oh!

Conversation

vic1707 commented Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

riley-williams commented Feb 28, 2025

Uh oh!

riley-williams commented Mar 14, 2025

Uh oh!

vic1707 commented Mar 14, 2025

Uh oh!

riley-williams commented Mar 14, 2025

Uh oh!

Uh oh!

vic1707 commented Feb 26, 2025 •

edited

Loading