chore: fix clippy issues

[akundaz]

starting to clean up the code, there's a lot more issues this is just a start

[0x416e746f6e]

could you pls split this into separate PR @akundaz?

[0x416e746f6e]

question:

personally, I very much dislike this kind of chains of operators.

code that this snippet replaces can be read easily even by non rust-adept person.

the code proposed, I need to stop for a couple of minutes, look up what of the each of methods does, and then maybe be ok with it.

question is: what does it improve?

[akundaz]

This is really just standard rust style. I understand it can be more difficult to read at first but once you're used to it it feels a lot faster to read code like this.

Rust is a very difficult language and when choosing it we need to be aware of the tradeoff that it will be difficult for non-rust engineers to read and write it. Most other popular languages are written in a procedural style but rust pulls a lot of ideas from functional programming and it's very common to see code like this.

Immutability is also critical to writing clean and easy to read rust code. If I see the mut keyword I have to be aware of later on when the value changes. In this diff I was trying to get closer to that -- there was only one append after this and I was planning on getting rid of it in a follow-up PR. If there's no mut I can be confident that the value doesn't change after being constructed and this lets everyone read the code significantly faster.

Regarding learning the chain methods -- this is part of learning rust, and I would expect anyone wanting to learn the language to become proficient using the iterator methods, along with the option and result methods and lots of other rust-specific stuff.

Here is a quick example from elsewhere in the codebase:

pub(crate) fn get_all_local_ip_addresses() -> Vec<IpAddr> {
    let mut ips = Vec::new();

    for interface in datalink::interfaces() {
        for ip in &interface.ips {
            ips.push(ip.ip());
        }
    }

    ips
}

This should be rewritten as:

pub(crate) fn get_all_local_ip_addresses() -> Vec<IpAddr> {
    datalink::interfaces()
        .into_iter()
        .flat_map(|interface| interface.ips.into_iter().map(|ip| ip.ip()))
        .collect()
}

This will probably look more difficult to read than the first snippet if you're new to Rust, since this first style looks similar to other languages. But in the second snippet, the data flow is more clear, it's immediately obvious that there are no side effects, and you don't need to think about mutable state (the vector that was originally there). It also forces the programmer to think about these things when writing their code, which prevents bugs in the long run that would arise from sprawling side effects and mutability.

[0x416e746f6e]

good point about mutability. but my point was about cognitive load for the reader (who is not necessarily rust-adept).

from snippets above that you provide I need one glance to understand what's going on in the first one. there's iteration (cognitive load == 1), then nested iteration (cognitive load == 2), then collecting of the IPs (cognitive load == 3).

in the second one we have iterator (cl == 1), then flat map (cl == 2), then interface IPs iterator (cl == 3), then simple map (cl == 4), then finally collect (cl == 5).

so, we get +2 CL points to achieve what?

i.m.o. unless there are indeed mutability concerns that would tip the balance over to writing complex pipelines, good old for loops are better off. I did love to write complicated python comprehensions some time ago, until I began to stumble over my own code (it hurts when instead of having a glance at code, you really have to think it through to understand what it does).

[akundaz]

Cognitive load is really subjective and depends in this case on rust proficiency. Functions like collect and map don't add much load. And you didn't count initializing the empty vec and returning it in the first snippet. I understand the concern that someone not familiar with rust will have a harder time with this but that's really a tradeoff that we're making in exchanges for benefits that rust provides.

Python list comprehensions are not the same as this. In python this would look something like

[ip.ip() for ip in interface.ips for interface in datalink.interfaces()]

which is ok for quick little snippets but it quickly becomes unwieldly if you try to add more logic. This is not the case in rust at all. I actually think writing pipelines is better with a more functional style.

This is what I want use to achieve:

• Clear data pipelines -- in my second snippet, it's easy to see the flow datalink::interfaces() -> flat_map indicating multiple values per interface -> iterate over ips -> call ip()
• This is a small example, but this design is more scalable. It's easy to add filter or filter_map or some other iterator function at a precise step in the pipeline. If you need side effects, you put a for_each in there and the reader knows that's where the side effects are. Breaking down pipelines into steps like this helps with testability too
• I want us to get used to reading and writing code like this so we start thinking about concepts like immutability, side effects when writing code, otherwise we struggle when it matters more. Also having all the code in a similar style helps readability

I will also try to find a larger example in this codebase and open a PR to show the difference it can make

[0x416e746f6e]

could you pls split this into separate PR @akundaz?

yes, after looking through: please do split into separate PRs.

• changes about dropping unnecessary clones are good, and thanks for them - I just didn't have time for these optimisations
• some of the changes that improve readability of config-related code are also good.

however there are changes that reduce readability for the sake of lower LoC metric + plus some of the changes that are making constraining architectural decisions way ahead of time => those I do not agree with.

^-- fusing all of these changes makes it hard to review. hence the ask

[akundaz]

I'm splitting it up so that just the linter changes are in this PR. It's also broken down by commit so you can see the progression and skip reviewing some of this.

[0x416e746f6e]

I'm splitting it up so that just the linter changes are in this PR. It's also broken down by commit so you can see the progression and skip reviewing some of this.

thanks.

can we please revert module inception? (and disable the clippy rule that's responsible for it).

having multiple mod.rs files that contain important logic is getting in the way of maintenance. (if I have several files open in IDE, all of which are called mod.rs it's going to be a hell of jumping back and forth between them until I find the right one).

[akundaz]

can we please revert module inception? (and disable the clippy rule that's responsible for it).

having multiple mod.rs files that contain important logic is getting in the way of maintenance. (if I have several files open in IDE, all of which are called mod.rs it's going to be a hell of jumping back and forth between them until I find the right one).

Hmm I don't want to disable this clippy rule, people will likely follow standard rust conventions when adding new modules just out of habit, so we'd end up with inconsistencies in the code organization.

The modern (or newer) way of organizing modules looks like this
Old:

src/
├── main.rs
└── foo/
    ├── mod.rs
    └── bar.rs

New:

src/
├── main.rs
├── foo.rs      # Module declaration file
└── foo/
    └── bar.rs  # Submodule

How does that sound to you? I added a new commit so you can see what the new directory structure would look like (no mod.rs files at all)

[0x416e746f6e]

question:

what is the point of this move?

I mean: before, all that was related to proxy_http was in proxy_http subdirectory. now, 80% is in crate root and 20% is in subdir. what does it improve?

if this is some other kind of clippy dogma, then I don't buy it.

e.g. I am looking how reth repo is organised and I don't see such kind of confusion there.