Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add problem: ByNamePackagePrefixedWithNumber #122

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/problem/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ pub mod npv_161;
pub mod npv_162;
pub mod npv_163;

pub mod npv_170;

#[derive(Clone, Display, EnumFrom)]
pub enum Problem {
/// NPV-100: attribute is not defined but it should be defined automatically
Expand Down Expand Up @@ -123,6 +125,9 @@ pub enum Problem {
NewTopLevelPackageShouldBeByNameWithCustomArgument(
npv_163::NewTopLevelPackageShouldBeByNameWithCustomArgument,
),

/// NPV-170: by-name package cannot be number prefixed
ByNamePackegPrefixedWithNumber(npv_170::ByNamePackegPrefixedWithNumber),
}

fn indent_definition(column: usize, definition: &str) -> String {
Expand Down
25 changes: 25 additions & 0 deletions src/problem/npv_170.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
use std::fmt;

use derive_new::new;
use relative_path::RelativePathBuf;

#[derive(Clone, new)]
pub struct ByNamePackegPrefixedWithNumber {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed this typo Packeg

#[new(into)]
package_name: String,
#[new(into)]
relative_package_dir: RelativePathBuf,
}

impl fmt::Display for ByNamePackegPrefixedWithNumber {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
let Self {
package_name,
relative_package_dir,
} = self;
write!(
f,
r#"- {relative_package_dir}: Attribute `{package_name}` should not be number-prefixed. It is suggestet to `"`-wrap this name"#
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestet

typo

"-wrap this name

I had to read that a couple of times before realizing what was meant. I think I'd prefer if it said something like "Wrap the name in quotes." However, it wasn't too clear to me when reading NixOS/nixpkgs#333281 if wrapping in quotes is preferred over underscore prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what the recommendation should be. This was a best guess. I'd happily take direction on what the warning should explicitly be.

Copy link
Member

@willbush willbush Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh looking at this again, realized they're not talking about the pname but the directory name for the package (which is what this checks 😂). Can you instead only recommend the _ prefix?

$ pwd
/home/will/code/external/nixpkgs/pkgs/by-name

$ find . -type d -name '_*'
./_1
./_1/_1oom
./_1/_1fps
./_2
./_2/_2ship2harkinian
./_4
./_4/_4ti2
./_4/_4th
./_4/_4d-minesweeper
./_6
./_6/_64gram
./_9
./_9/_9base
./_9/_9menu
./mi/micro/tests/_001-hello-expect
./_0
./_0/_0xpropo
./_0/_0ver

$ rg "pname = \"1"
_1/_1fps/package.nix
8:  pname = "1fps";

_1/_1oom/package.nix
19:  pname = "1oom";

$ rg "pname = \"_"

)
}
}
31 changes: 23 additions & 8 deletions src/structure.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@ use lazy_static::lazy_static;
use regex::Regex;
use relative_path::RelativePathBuf;

use crate::problem::{npv_109, npv_110, npv_111, npv_140, npv_141, npv_142, npv_143, npv_144};
use crate::problem::{
npv_109, npv_110, npv_111, npv_140, npv_141, npv_142, npv_143, npv_144, npv_170,
};
use crate::references;
use crate::validation::{self, ResultIteratorExt, Validation::Success};
use crate::NixFileStore;
Expand All @@ -16,8 +18,9 @@ pub const BASE_SUBPATH: &str = "pkgs/by-name";
pub const PACKAGE_NIX_FILENAME: &str = "package.nix";

lazy_static! {
static ref SHARD_NAME_REGEX: Regex = Regex::new(r"^[a-z0-9_-]{1,2}$").unwrap();
static ref PACKAGE_NAME_REGEX: Regex = Regex::new(r"^[a-zA-Z0-9_-]+$").unwrap();
static ref SHARD_NAME_REGEX: Regex = Regex::new(r"^((_[0-9])|([a-z][a-z0-9_-]?))$").unwrap();
static ref PACKAGE_NAME_REGEX: Regex =
Regex::new(r"^((_[0-9])|[a-zA-Z])[a-zA-Z0-9_-]*$").unwrap();
Comment on lines +21 to +23
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
static ref SHARD_NAME_REGEX: Regex = Regex::new(r"^((_[0-9])|([a-z][a-z0-9_-]?))$").unwrap();
static ref PACKAGE_NAME_REGEX: Regex =
Regex::new(r"^((_[0-9])|[a-zA-Z])[a-zA-Z0-9_-]*$").unwrap();
static ref SHARD_NAME_REGEX: Regex = Regex::new(r"^[a-z_-][a-z0-9_-]?$").unwrap();
static ref PACKAGE_NAME_REGEX: Regex = Regex::new(r"^[a-zA-Z_-][a-zA-Z0-9_-]*$").unwrap();

I think this is a bit simpler to understand

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually don't even need a new error type for this, because we can just update the message for when these regexes don't match here:

"- {relative_shard_path}: Invalid directory name \"{shard_name}\", must be at most 2 ASCII characters consisting of a-z, 0-9, \"-\" or \"_\".",
and
"- {relative_package_dir}: Invalid package directory name \"{package_name}\", must be ASCII characters consisting of a-z, A-Z, 0-9, \"-\" or \"_\".",

}

/// Deterministic file listing so that tests are reproducible.
Expand Down Expand Up @@ -137,11 +140,23 @@ fn check_package(
} else {
let package_name_valid = PACKAGE_NAME_REGEX.is_match(&package_name);
let result = if !package_name_valid {
npv_141::InvalidPackageDirectoryName::new(
package_name.clone(),
relative_package_dir.clone(),
)
.into()
if package_name
willbush marked this conversation as resolved.
Show resolved Hide resolved
.chars()
.next()
.is_some_and(|c| c.is_ascii_digit())
{
npv_170::ByNamePackegPrefixedWithNumber::new(
package_name.clone(),
relative_package_dir.clone(),
)
.into()
} else {
npv_141::InvalidPackageDirectoryName::new(
package_name.clone(),
relative_package_dir.clone(),
)
.into()
}
} else {
Success(())
};
Expand Down
1 change: 1 addition & 0 deletions tests/by-name-numprefix/default.nix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
import <test-nixpkgs> { root = ./.; }
2 changes: 2 additions & 0 deletions tests/by-name-numprefix/expected
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- pkgs/by-name/_1/10foo: Attribute `10foo` should not be number-prefixed. It is suggestet to `"`-wrap this name
This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
4 changes: 4 additions & 0 deletions tests/by-name-numprefix/pkgs/by-name/_1/10foo/package.nix
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something to think about is how users are likely to run into a problem. Since users will know that the first two chars of the shard need to match the attribute name, _1/10foo is rather unlikely, so instead it's better to make sure it triggers on 10/10foo.

Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{ someDrv }:
SomeDrv
# If we caused an actual Nix failure
# builtins.trace "This should be on stderr!" throw "This is an error!"
willbush marked this conversation as resolved.
Show resolved Hide resolved