refactor: cleanup imports and remove globs

[DrPeterVanNostrand]

1. Removes glob imports everywhere except in two cases:

when re-exporting in lib.rs/mod.rs files:

mod mod_1;
mod mod_2;

pub use mod_1::*;
pub use mod_2::*;

in unit test submodules:

#[cfg(test)]
mod tests {
    use super::*;
}

2. Enforces the import order:

// Only in lib.rs/mod.rs files
#[macro_use]
pub mod public_module_containing_macros;

// Only in lib.rs/mod.rs files
#[macro_use]
mod private_module_containing_macros;

// Only as the first line of an inner-file submodule e.g. `mod tests`
use super::*;

use std::something;

pub use external_crate::something_to_export;

use external_crate::something_to_use;

pub use crate::something_to_export;

use crate::something_to_use;

pub mod public_module;

mod private_module;

pub use some_module::something_to_export;

use some_module::something_to_use;

3. Removes unnecessary/repeated namespace operators:

// Changed code like this:
let x = std::io::copy(...);
let y = std::io::copy(...);
let z = std::io::copy(...);

// To this:
use std::io;

let x = io::copy(...);
let y = io::copy(...);
let z = io::copy(...);

4. Uses nested imports (except in std library imports):

use std::io;
use std::fs;

use crate::{mod_1::thing, mod_2::other_thing};

5. Removes duplicate imports, for example in code like:

use something;

#[cfg(test)]
mod tests {
    use something;
}

6. Replaces hard to read imports like:

use super::super::something;

7. Removes enum variant imports:

// Changes something like this:
use some_crate::SomeEnum::SomeVariant;

let x = SomeVariant;

// To this:
use some_crate::SomeEnum;

let x = SomeEnum::SomeVariant;

[cryptonemo]

@DrPeterVanNostrand Nice -- I should be done reviewing this tomorrow. Based on this, I'm anticipating a very unfriendly rebase on my local branch work (hopefully can be sorted out tomorrow too though).

[vmx]

1. Removes glob imports everywhere except in two cases:

I'm in the "almost never use" globs camp. I find code easier to navigate if I can grep for things. Even for those two cases:

when re-exporting in lib.rs/mod.rs files:

If things are deeply nested, things "bubble" up automatically and you might accidentally put something into the top-level API. I also think the top-level API shouldn't cluttered. I'm not sure if that's best practice, but what I sometimes do is putting the widely used things into the top-level, but leaving very specific things (which might be still useful, hence are public) within a module.

What I also did in the past was putting a generic version of some type into the module, but putting a specific version on the top level. So it's kind of a re-export with the same name.

Though there are cases where a glob might be appropriate, though I wouldn't say it's generally the case.

in unit test submodules:

I also usually don't do that as I like copy&pastable tests. Often tests are the best documentation a library has and they are a good starting point. Though there I can totally see that one prefers using a glob, so I'm not oppose if someone is using a glob there.

---

In the PR I've seen that you directly import functions. I usually follow the rust style guidelines where you don't import functions, but only their module. E.g:

use rand::Rng;
…
let bytes = rand::thread_rng().gen::<[u8; 32]>();


// and not
use rand::{thread_rng, Rng};
…
let bytes = thread_rng().gen::<[u8; 32]>();

[cryptonemo]

use rand::Rng;
…
let bytes = rand::thread_rng().gen::<[u8; 32]>();


// and not
use rand::{thread_rng, Rng};
…
let bytes = thread_rng().gen::<[u8; 32]>();

I'm ok with whatever convention we go with, as I think striving for consistency is most important when it comes to style -- but I find the former more readable, as the context is more quickly found.

[cryptonemo]

Only ~25 files left 😅

So far, this all looks very straight-forward. Like I said, there are some inconsistencies here and there, but overall looks much better to me.

[dignifiedquire]

[haven't reviewed all files in detail] general rules and things I saw look good to me, I'll leave the detailed check to @cryptonemo

[vmx]

I should've noted that my comment was about my preference and how I do it and not a request that we must do it this way. I certainly don't wan to to block anything and this PR improves things for sure.

[cryptonemo]

Looks good

[dignifiedquire]

@DrPeterVanNostrand are you still changing things here, or can this be merged?

[cryptonemo]

@DrPeterVanNostrand are you still changing things here, or can this be merged?

@DrPeterVanNostrand indicated on the call that some changes were incoming, so I believe we're waiting on a last push.

[DrPeterVanNostrand]

@cryptonemo

Sorry, pushing my changes this morning

[DrPeterVanNostrand]

Changes have been pushed.

[cryptonemo]

Looks good, thanks! (Yay for github, remembering my previously reviewed files)

[DrPeterVanNostrand]

Awesome, thank you for reviewing it again. Should I merge now, or wait for other PR's to merge?

[cryptonemo]

Awesome, thank you for reviewing it again. Should I merge now, or wait for other PR's to merge?

Merge at will!

[DrPeterVanNostrand]

@cryptonemo thanks, doing now

[dbshch]

This commit leads to a problem that on ARM platform, the mod sha256_intrinsics can't be imported.

error[E0432]: unresolved import crate::sha256_intrinsics
--> sha2raw/src/platform.rs:1:13
|
1 | use crate::{sha256_intrinsics, sha256_utils};
| ^^^^^^^^^^^^^^^^^ no sha256_intrinsics in the root

[cryptonemo]

@dbshch Thank you for noting this -- we are going to be looking into the ARM platform in the future. Can you create a top-level issue for what you're seeing on this for tracking it?