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 all 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
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ The following checks are performed when calling the binary:
- `pkgs/by-name` must only contain subdirectories of the form `${shard}/${name}`, called _package directories_.
- The `name`'s of package directories must be unique when lowercased.
- `name` is a string only consisting of the ASCII characters `a-z`, `A-Z`, `0-9`, `-` or `_`.
- `name` only starts with an ASCII character `a-z`, `A-Z`, `-` or `_`.
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
- `name` only starts with an ASCII character `a-z`, `A-Z`, `-` or `_`.
- `name` only starts with an ASCII character `a-z`, `A-Z` or `_`.

It cannot start with -. - will be parsed to a negation operator.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this needs to be done in this same PR, since it's technically a separate problem, but yeah this would be good too :)

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this needs to be done in this same PR, since it's technically a separate problem, but yeah this would be good too :)

It's not allowed in the regex already.

- `shard` is the lowercased first two letters of `name`, expressed in Nix: `shard = toLower (substring 0 2 name)`.
- Each package directory must contain a `package.nix` file and may contain arbitrary other files.

Expand Down
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,
"- {relative_package_dir}: Attribute `{package_name}` should not be number-prefixed. Prefix with `_`, or wrap in quotes"
)
}
}
27 changes: 19 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 \"_\".",

Copy link
Member

Choose a reason for hiding this comment

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

static ref PACKAGE_NAME_REGEX: Regex = Regex::new(r"^[a-zA-Z_-][a-zA-Z0-9_-]*$").unwrap();

This allows _abc and even ___ as package name.

}

/// Deterministic file listing so that tests are reproducible.
Expand Down Expand Up @@ -137,11 +140,19 @@ 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.starts_with(|c: char| 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. Prefix with `_`, or wrap in quotes
This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.
1 change: 1 addition & 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 @@
{ someDrv }: SomeDrv