mirror of
https://github.com/pnpm/pnpm.git
synced 2026-07-23 06:02:50 -04:00
refactor: no star imports (#364)
https://claude.ai/code/session_011yRgFgrJbtbneNyrvTJp32 --------- Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
@@ -149,6 +149,12 @@ or treating the red as acceptable.
|
||||
scalar `assert_eq!`.
|
||||
- Follow [Rust API Guidelines](https://rust-lang.github.io/api-guidelines/naming.html)
|
||||
for naming.
|
||||
- **No star imports inside module bodies.** Write `use super::{Foo, bar}`
|
||||
instead of `use super::*;`, and the same for any other glob whose
|
||||
target is a module you control. Two forms stay allowed: external-crate
|
||||
preludes such as `use rayon::prelude::*;` and root-of-module
|
||||
re-exports such as `pub use submodule::*;` in a `lib.rs`. See the
|
||||
"No star imports" section in `CODE_STYLE_GUIDE.md`.
|
||||
|
||||
### Preserve existing method chains
|
||||
|
||||
|
||||
@@ -75,6 +75,43 @@ use std::{path::PathBuf, sync::Arc};
|
||||
use std::os::unix::fs::PermissionsExt;
|
||||
```
|
||||
|
||||
### No star imports
|
||||
|
||||
Avoid star (glob) imports inside the bodies of regular modules. Import items explicitly by name everywhere except the two cases noted below. The rule applies to production code, tests, integration tests, build scripts, and developer tooling under `tasks/`.
|
||||
|
||||
The two exceptions are:
|
||||
|
||||
1. **External-crate preludes**, such as `use rayon::prelude::*;` or `use assert_cmd::prelude::*;`. The upstream crate has already curated which items are intended to be glob-imported, so listing them out by hand creates a maintenance burden the moment the upstream prelude changes. Use the prelude in the form the crate documents.
|
||||
2. **Re-exports at the root of a module or crate**, such as `pub use submodule::*;` in `lib.rs`. These are part of the public surface that the crate intentionally exposes, and listing the items individually duplicates information that already lives in the submodule.
|
||||
|
||||
Star imports inside a module body are the case worth banning. They make it hard to tell where a name comes from, they hide accidental shadowing, and `use super::*;` is especially harmful in tests. The form pulls every privately imported item from the outer module into scope, so an import the production code no longer uses can keep compiling indefinitely as long as some test still references it. Removing dead imports becomes guesswork.
|
||||
|
||||
```rust
|
||||
// Bad
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
}
|
||||
|
||||
// Good
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::{ParsedThing, parse_thing};
|
||||
}
|
||||
```
|
||||
|
||||
```rust
|
||||
// Allowed (external-crate prelude)
|
||||
use rayon::prelude::*;
|
||||
use assert_cmd::prelude::*;
|
||||
```
|
||||
|
||||
```rust
|
||||
// Allowed (root-of-module re-export)
|
||||
pub use comver::*;
|
||||
pub use load_lockfile::*;
|
||||
```
|
||||
|
||||
### Generic Parameter Naming
|
||||
|
||||
Use **descriptive names** for type parameters, not single letters:
|
||||
|
||||
@@ -108,7 +108,7 @@ impl AddArgs {
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use super::AddDependencyOptions;
|
||||
use pacquet_package_manifest::DependencyGroup;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
|
||||
@@ -75,7 +75,7 @@ impl InstallArgs {
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use super::InstallDependencyOptions;
|
||||
use pacquet_package_manifest::DependencyGroup;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
|
||||
@@ -82,7 +82,7 @@ where
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use super::call_load_lockfile;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -507,9 +507,16 @@ fn strip_dash_suffix(name: &str) -> String {
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use super::{
|
||||
EnsureFileError, ensure_file, file_equals_bytes, is_transient_rename_error,
|
||||
rename_with_retry, temp_path_for,
|
||||
};
|
||||
use std::{fs, io, path::Path};
|
||||
use tempfile::tempdir;
|
||||
|
||||
#[cfg(unix)]
|
||||
use super::{EMFILE, ENFILE, retry_on_fd_pressure};
|
||||
|
||||
/// New-file path: contents land on disk. Mode handling is covered
|
||||
/// separately in `unix_mode_is_applied_on_new_files`.
|
||||
#[test]
|
||||
|
||||
@@ -56,7 +56,7 @@ impl From<ComVer> for String {
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use super::ComVer;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -34,7 +34,7 @@ impl<const MAJOR: u16> TryFrom<ComVer> for LockfileVersion<MAJOR> {
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use super::{ComVer, LockfileVersion, LockfileVersionError};
|
||||
use pipe_trait::Pipe;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
|
||||
@@ -89,7 +89,8 @@ impl From<PkgName> for String {
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use super::{ParsePkgNameError, PkgName};
|
||||
use pipe_trait::Pipe;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -11,7 +11,8 @@ pub type ParsePkgNameVerError = ParsePkgNameSuffixError<SemverError>;
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use super::{ParsePkgNameVerError, PkgNameVer};
|
||||
use node_semver::Version;
|
||||
use pipe_trait::Pipe;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
|
||||
@@ -36,7 +36,7 @@ impl PkgNameVerPeer {
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use super::PkgNameVerPeer;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
fn name_peer_ver(name: &str, peer_ver: &str) -> PkgNameVerPeer {
|
||||
|
||||
@@ -81,7 +81,8 @@ impl From<PkgVerPeer> for String {
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use super::{ParsePkgVerPeerError, PkgVerPeer};
|
||||
use node_semver::Version;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
fn assert_ver_peer<Ver, Peer>(received: PkgVerPeer, expected_version: Ver, expected_peer: Peer)
|
||||
|
||||
@@ -43,7 +43,8 @@ impl ProjectSnapshot {
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use super::{ProjectSnapshot, ResolvedDependencySpec};
|
||||
use pacquet_package_manifest::DependencyGroup;
|
||||
use pretty_assertions::assert_eq;
|
||||
use text_block_macros::text_block;
|
||||
|
||||
|
||||
@@ -98,9 +98,13 @@ impl From<LockfileResolution> for ResolutionSerde {
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use super::{
|
||||
DirectoryResolution, GitResolution, LockfileResolution, RegistryResolution,
|
||||
TarballResolution,
|
||||
};
|
||||
use crate::serialize_yaml;
|
||||
use pretty_assertions::assert_eq;
|
||||
use ssri::Integrity;
|
||||
use text_block_macros::text_block;
|
||||
|
||||
fn integrity(integrity_str: &str) -> Integrity {
|
||||
|
||||
@@ -37,7 +37,8 @@ impl Lockfile {
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use super::SaveLockfileError;
|
||||
use crate::Lockfile;
|
||||
use pretty_assertions::assert_eq;
|
||||
use tempfile::tempdir;
|
||||
use text_block_macros::text_block;
|
||||
|
||||
@@ -130,7 +130,8 @@ impl From<PkgVerPeer> for SnapshotDepRef {
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use super::{SnapshotDepRef, looks_like_alias};
|
||||
use crate::{PkgName, PkgNameVerPeer, PkgVerPeer};
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
fn pkg_name(s: &str) -> PkgName {
|
||||
|
||||
@@ -173,11 +173,17 @@ where
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use super::default_store_dir;
|
||||
use crate::test_env_guard::EnvGuard;
|
||||
use pacquet_store_dir::StoreDir;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::env;
|
||||
|
||||
#[cfg(windows)]
|
||||
use super::{default_store_dir_windows, get_drive_letter};
|
||||
#[cfg(windows)]
|
||||
use std::path::Path;
|
||||
|
||||
fn display_store_dir(store_dir: &StoreDir) -> String {
|
||||
store_dir.display().to_string().replace('\\', "/")
|
||||
}
|
||||
|
||||
@@ -316,8 +316,10 @@ mod tests {
|
||||
use pretty_assertions::assert_eq;
|
||||
use tempfile::tempdir;
|
||||
|
||||
use super::*;
|
||||
use crate::test_env_guard::EnvGuard;
|
||||
use super::{NodeLinker, Npmrc, PackageImportMethod, fs};
|
||||
use crate::{custom_deserializer::default_store_dir, test_env_guard::EnvGuard};
|
||||
use pacquet_store_dir::StoreDir;
|
||||
use pipe_trait::Pipe;
|
||||
|
||||
fn display_store_dir(store_dir: &StoreDir) -> String {
|
||||
store_dir.display().to_string().replace('\\', "/")
|
||||
|
||||
@@ -64,7 +64,8 @@ impl NpmrcAuth {
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use super::NpmrcAuth;
|
||||
use crate::Npmrc;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -194,8 +194,11 @@ pub fn workspace_root_or(start: &Path) -> PathBuf {
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use super::WorkspaceSettings;
|
||||
use crate::{NodeLinker, Npmrc};
|
||||
use pacquet_store_dir::StoreDir;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::{fs, path::Path};
|
||||
|
||||
#[test]
|
||||
fn parses_common_settings_from_yaml() {
|
||||
|
||||
@@ -115,11 +115,13 @@ pub fn build_package_snapshot(
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use super::{BuildSnapshotError, build_package_snapshot, registry_package_key};
|
||||
use node_semver::Version;
|
||||
use pacquet_lockfile::{LockfileResolution, PkgName, PkgVerPeer};
|
||||
use pacquet_registry::{PackageDistribution, PackageVersion};
|
||||
use pretty_assertions::assert_eq;
|
||||
use ssri::Integrity;
|
||||
use std::collections::HashMap;
|
||||
|
||||
fn integrity(s: &str) -> Integrity {
|
||||
s.parse().expect("parse integrity string")
|
||||
|
||||
@@ -157,7 +157,7 @@ where
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use super::{Install, InstallError};
|
||||
use pacquet_npmrc::Npmrc;
|
||||
use pacquet_package_manifest::{DependencyGroup, PackageManifest};
|
||||
use pacquet_registry_mock::AutoMockInstance;
|
||||
|
||||
@@ -159,14 +159,14 @@ impl<'a> InstallPackageFromRegistry<'a> {
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use super::InstallPackageFromRegistry;
|
||||
use node_semver::Version;
|
||||
use pacquet_network::ThrottledClient;
|
||||
use pacquet_npmrc::Npmrc;
|
||||
use pacquet_store_dir::StoreDir;
|
||||
use pacquet_store_dir::{SharedVerifiedFilesCache, StoreDir};
|
||||
use pipe_trait::Pipe;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::fs;
|
||||
use std::path::Path;
|
||||
use std::{fs, path::Path};
|
||||
use tempfile::tempdir;
|
||||
|
||||
fn create_config(store_dir: &Path, modules_dir: &Path, virtual_store_dir: &Path) -> Npmrc {
|
||||
|
||||
@@ -310,8 +310,17 @@ fn clone_or_copy_link(state: &AtomicU8, source: &Path, target: &Path) -> io::Res
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use super::{
|
||||
LINK_STATE_CLONE, LINK_STATE_COPY, LINK_STATE_HARDLINK, LinkFileError, auto_link,
|
||||
clone_or_copy_link, is_call_error, is_cross_device, link_file,
|
||||
};
|
||||
use pacquet_npmrc::PackageImportMethod;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::{
|
||||
fs, io,
|
||||
path::{Path, PathBuf},
|
||||
sync::atomic::{AtomicU8, Ordering},
|
||||
};
|
||||
use tempfile::tempdir;
|
||||
|
||||
fn write_source(dir: &Path, name: &str, contents: &[u8]) -> PathBuf {
|
||||
|
||||
@@ -245,8 +245,9 @@ mod tests {
|
||||
use pretty_assertions::assert_eq;
|
||||
use tempfile::{NamedTempFile, tempdir};
|
||||
|
||||
use super::*;
|
||||
use super::{BundleDependencies, PackageManifest};
|
||||
use crate::DependencyGroup;
|
||||
use std::io::Write;
|
||||
|
||||
#[test]
|
||||
fn test_init_package_json_content() {
|
||||
|
||||
@@ -80,7 +80,7 @@ mod tests {
|
||||
use node_semver::Version;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
use super::*;
|
||||
use super::PackageVersion;
|
||||
use crate::package_distribution::PackageDistribution;
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -85,7 +85,9 @@ impl StoreDir {
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::StoreDir;
|
||||
use sha2::{Digest, Sha512};
|
||||
use std::path::PathBuf;
|
||||
|
||||
#[test]
|
||||
fn cas_file_path() {
|
||||
|
||||
@@ -334,9 +334,16 @@ fn verify_file_integrity(path: &Path, digest: &str, algo: &str) -> bool {
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use super::{VerifiedFilesCache, build_file_maps_from_index, check_pkg_files_integrity};
|
||||
use crate::{CafsFileInfo, PackageFilesIndex, StoreDir};
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::{fs, io::Write, time::SystemTime};
|
||||
use sha2::{Digest, Sha512};
|
||||
use std::{
|
||||
fs,
|
||||
io::Write,
|
||||
path::PathBuf,
|
||||
time::{SystemTime, UNIX_EPOCH},
|
||||
};
|
||||
use tempfile::tempdir;
|
||||
|
||||
/// Write `content` to the correct CAFS path under `store_dir` for
|
||||
|
||||
@@ -930,8 +930,11 @@ fn write_bool(w: &mut Vec<u8>, b: bool) {
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::{CafsFileInfo, PackageFilesIndex};
|
||||
use super::{
|
||||
DecodeError, EncodeError, EncodeState, FIRST_INNER_SLOT, PKG_FILES_INDEX_SLOT,
|
||||
RECORD_DEF_EXT_TYPE, SLOT_HI, encode_package_files_index, transcode_to_plain_msgpack,
|
||||
};
|
||||
use crate::{CafsFileInfo, PackageFilesIndex, SideEffectsDiff};
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::collections::HashMap;
|
||||
|
||||
|
||||
@@ -198,7 +198,7 @@ impl StoreDir {
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use super::StoreDir;
|
||||
use pipe_trait::Pipe;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::path::Path;
|
||||
|
||||
@@ -611,8 +611,10 @@ pub struct SideEffectsDiff {
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use super::{CafsFileInfo, GET_MANY_CHUNK, PackageFilesIndex, StoreIndex, store_index_key};
|
||||
use crate::StoreDir;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::collections::HashMap;
|
||||
use tempfile::tempdir;
|
||||
|
||||
fn sample_index() -> PackageFilesIndex {
|
||||
|
||||
@@ -1,10 +1,20 @@
|
||||
use pacquet_store_dir::{SharedVerifiedFilesCache, StoreIndex};
|
||||
use super::{
|
||||
DownloadTarballToStore, HttpStatusError, MemCache, NetworkError, PrefetchedCasPaths, RetryOpts,
|
||||
TarballError, VerifyChecksumError, allocate_tarball_buffer, extract_tarball_entries,
|
||||
fetch_and_extract_with_retry, is_transient_error, prefetch_cas_paths,
|
||||
};
|
||||
use pacquet_network::ThrottledClient;
|
||||
use pacquet_store_dir::{
|
||||
CafsFileInfo, PackageFilesIndex, SharedVerifiedFilesCache, StoreDir, StoreIndex,
|
||||
store_index_key,
|
||||
};
|
||||
use pipe_trait::Pipe;
|
||||
use pretty_assertions::assert_eq;
|
||||
use ssri::Integrity;
|
||||
use std::{collections::HashMap, io::Cursor, path::PathBuf, sync::Arc, time::Duration};
|
||||
use tar::Archive;
|
||||
use tempfile::{TempDir, tempdir};
|
||||
|
||||
use super::*;
|
||||
|
||||
fn integrity(integrity_str: &str) -> Integrity {
|
||||
integrity_str.parse().expect("parse integrity string")
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user