chore(transport): remove module_name_repetitions

[mxinden]

We allow the clippy lint module_name_repetitions:

neqo/neqo-transport/src/lib.rs

Line 7 in a758177

#![allow(clippy::module_name_repetitions)] // This lint doesn't work here.

We e.g. prefix various types in the ecn module with Ecn:

neqo/neqo-transport/src/ecn.rs

Lines 18 to 30 in 3001a3a

	/// The number of packets to use for testing a path for ECN capability.
	pub const ECN_TEST_COUNT: usize = 10;
	/// The number of packets to use for testing a path for ECN capability when exchanging
	/// Initials during the handshake. This is a lower number than [`ECN_TEST_COUNT`] to avoid
	/// unnecessarily delaying the handshake; we would otherwise double the PTO [`ECN_TEST_COUNT`]
	/// times.
	const ECN_TEST_COUNT_INITIAL_PHASE: usize = 3;
	/// The state information related to testing a path for ECN capability.
	/// See RFC9000, Appendix A.4.
	#[derive(Debug, PartialEq, Clone, Copy)]
	enum EcnValidationState {

As discussed in #2270 (comment), I suggest removing the allow and instead fix the module name repetition.

[mansf-osk]

I'll start working on this! Are there any places where we don't want to get rid of the module name repetitions? As far as I can see we allow them in a lot of modules, just going by this quick search.

Maybe it makes sense to remove the crate-wide allow attributes (as the one referenced above) and leave be or possibly add local allow's where it makes sense to keep the module name repetitions?

[mxinden]

@mansf-osk I suggest starting with a single module, e.g. neqo-transport/src/ecn.rs.

Small pull requests reduce the likelihood of merge conflicts and make reviewing easier.

Maybe it makes sense to remove the crate-wide allow attributes (as the one referenced above) and leave be or possibly add local allow's where it makes sense to keep the module name repetitions?

Happy for the crate-wide #![allow(clippy::module_name_repetitions)] to be replaced by the missing local allows with the first pull request.

[larseggert]

Just so I understand, we would rename e.g. EcnInfo to Ecn::Info, i.e. prefix with the module name? (Because otherwise Info is a bit generic.)

[mxinden]

It would be defined as:

// neqo-transport/src/ecn.rs

pub struct Info {
  // ...
}

and it would be used as:

// Some other file.

use ecn;

fn my_func() {
  let ecn_info = ecn::Info::new();
}

Just like in the standard library where it is std::io::Error and not std::io::IoError.

Does that make sense @larseggert?

[mansf-osk]

I'd like some feedback on the following before eventually tackling more of this refactor!

The way we did it in #2311 (like in Max's example above) we would eventually end up losing lots of explicit imports that we have right now. I'm not that big a fan of that personally, so I'd like to use aliases instead, as is also suggested in the discussions around that lint's RFC. I especially like the following sentiment in that comment:

"Essentially, this pushes resolving overlaps between names to the client of an API (where the overlaps are actually created) rather than the provider of an API (who would otherwise have to guess at which overlaps will occur)."

Going by the example above it would look like that:

use ecn::{Info as EcnInfo, Config as EcnConfig}

fn my_func() {
    let ecn_info = EcnInfo::new();
    let ecn_config = EcnConfig::new();
    // ...
}

That way we still have the prefix for clarity where we consume the API while also adhering to the lint and not losing all our explicit imports. As I see it this is the most canonical way to implement that lint.

Thoughts?

[larseggert]

WFM, but it requires the importer to not be lazy and leave the import named as-is. If the importer was lazy, your example would look like this, which I think would be bad:

use ecn::{Info, Config}

fn my_func() {
    let ecn_info = Info::new();
    let ecn_config = Config::new();
    // ...
}

[mxinden]

The way we did it in #2311 (like in Max's example above) we would eventually end up losing lots of explicit imports that we have right now. I'm not that big a fan of that personally, so I'd like to use aliases instead, as is also suggested in the discussions around that lint's RFC.

I don't think "losing lots of explicit imports" is a problem.

In my eyes importing a module (e.g. use ecn;) only is simpler,

use ecn;

fn my_func() {
  let ecn_info = ecn::Info::new();
  let ecn_config = ecn::Config::new();
}

than the aliasing:

use ecn::{Info as EcnInfo, Config as EcnConfig}

fn my_func() {
    let ecn_info = EcnInfo::new();
    let ecn_config = EcnConfig::new();
}

[mansf-osk]

Thanks for the feedback, I'll keep it as is then. Will put the refactor in my backlog and chip away at it when I have bandwidth.