Add problem: ByNamePackagePrefixedWithNumber #122

Ben-PH · 2024-10-29T08:00:15Z

Reference issue: #107

This is first time putting together something of substance for this repo.

Ben-PH · 2024-10-29T09:18:19Z

@philiptaron I think I have the bulk of the idea here.

copy one of the tests/ directories. rename things to trigger the new vet-check
creat a new module in problem module templated by a similar problem
have the Display match the expected file in the new tests/ dir-entry

...I'm just not sure about this: https://github.com/NixOS/nixpkgs-vet/actions/runs/11570703378/job/32207036558?pr=122#step:4:907 (I see that I should probably be going by the invalid-shard-name test. That resolves the local test failure)

...and feedback on what the error message should actually be (I'm pretty sure the suggestion is wrong)

willbush

Thanks for your contributions!

willbush · 2024-10-30T06:11:43Z

src/problem/npv_170.rs

+        } = self;
+        write!(
+            f,
+            r#"- {relative_package_dir}: Attribute `{package_name}` should not be number-prefixed. It is suggestet to `"`-wrap this name"#


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.

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.

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 = \"_"

tests/by-name-numprefix/pkgs/by-name/_1/10foo/package.nix

src/structure.rs

willbush · 2024-10-30T06:36:13Z

@philiptaron @infinisil forgot how where landed on how clean commit history should be for feature branches since we merge commit them into main? For example, should requested fix changes on a PR be rebased?

willbush · 2024-10-30T09:02:23Z

@Ben-PH Just thought of the readme mentioning checks. Might be worth updating that too?

willbush · 2024-10-30T09:49:17Z

src/problem/npv_170.rs

+use relative_path::RelativePathBuf;
+
+#[derive(Clone, new)]
+pub struct ByNamePackegPrefixedWithNumber {


Just noticed this typo Packeg

Aleksanaa · 2024-11-01T09:53:36Z

README.md

@@ -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 `_`.


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.

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 :)

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.

infinisil

Thank you! Left some comments :)

infinisil · 2024-11-01T10:31:33Z

tests/by-name-numprefix/pkgs/by-name/_1/10foo/package.nix

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.

infinisil · 2024-11-01T10:33:02Z

README.md

@@ -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 `_`.


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 :)

infinisil · 2024-11-01T10:37:24Z

src/structure.rs

+    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();


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

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:

nixpkgs-vet/src/problem/npv_110.rs

Line 19 in 02db8e4

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

and

nixpkgs-vet/src/problem/npv_141.rs

Line 22 in 02db8e4

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

Ben-PH added 7 commits October 29, 2024 08:54

Initial NixOS#107 work

5006491

Update shard/package name sanatisation regex

85a1b53

Add test

af703c9

cargo fmt

fd4a72f

Rename dir: 10 -> _1

e693e74

add leading digit check

884c807

Cleanup err-msg and match(ish) it with expected

05bafef

Ben-PH added 2 commits October 29, 2024 10:36

Remodel test based on invalid name test

c20b06e

treefmt

7f3e6ce

Ben-PH marked this pull request as ready for review October 29, 2024 10:04

willbush requested changes Oct 30, 2024

View reviewed changes

Review followup

932f6ef

Ben-PH added 2 commits October 30, 2024 10:17

Fix CI error

837cba7

Update README

2e54489

willbush self-requested a review October 30, 2024 09:48

willbush requested changes Oct 30, 2024

View reviewed changes

willbush requested review from philiptaron and infinisil October 30, 2024 09:52

Aleksanaa reviewed Nov 1, 2024

View reviewed changes

infinisil requested changes Nov 1, 2024

View reviewed changes

infinisil mentioned this pull request Nov 1, 2024

Replace lazy_static with std::sync::LazyLock #123

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add problem: ByNamePackagePrefixedWithNumber #122

Add problem: ByNamePackagePrefixedWithNumber #122

Ben-PH commented Oct 29, 2024

Ben-PH commented Oct 29, 2024 •

edited

Loading

willbush left a comment

willbush Oct 30, 2024

Ben-PH Oct 30, 2024

willbush Oct 30, 2024 •

edited

Loading

willbush commented Oct 30, 2024 •

edited

Loading

willbush commented Oct 30, 2024

willbush Oct 30, 2024

Aleksanaa Nov 1, 2024

infinisil Nov 1, 2024

Aleksanaa Nov 1, 2024

infinisil left a comment

infinisil Nov 1, 2024

infinisil Nov 1, 2024

infinisil Nov 1, 2024

infinisil Nov 1, 2024

	- `name` only starts with an ASCII character `a-z`, `A-Z`, `-` or `_`.
	- `name` only starts with an ASCII character `a-z`, `A-Z` or `_`.

Add problem: ByNamePackagePrefixedWithNumber #122

Are you sure you want to change the base?

Add problem: ByNamePackagePrefixedWithNumber #122

Conversation

Ben-PH commented Oct 29, 2024

Ben-PH commented Oct 29, 2024 • edited Loading

willbush left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willbush Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

willbush commented Oct 30, 2024 • edited Loading

willbush commented Oct 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infinisil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ben-PH commented Oct 29, 2024 •

edited

Loading

willbush Oct 30, 2024 •

edited

Loading

willbush commented Oct 30, 2024 •

edited

Loading