This could use a docstring explaining the purpose of the extra_nix_path parameter and how to interpret the boolean return value.

Added! Also renamed the parameter to eval_accessible_paths: Vec<&Path> to be more self-explanatory.

Nitpick: generally you don't see impl blocks split like this unless there's generic type parameters or trait bounds requiring you to do so.

Yeah, I was thinking that too, but I intentionally split it to have a better grouping: The long initialization vs the static utility functions merely related to the type. I'd prefer to leave it like this unless there's a good reason not to do that

Detecting errors by checking whether this writer is empty is an elegant solution, but it may not be the most obvious one. Rather than coming up with a different solution you could probably make this more obvious with a more explicit name like error_writer and a comment explaining that this writer will only be empty if no errors were encountered while parsing nixpkgs.

(there is a comment below), but I renamed it to error_writer, also changed check_nixpkgs to only take an io::Write, internally wrapping it with ErrorWriter. Makes more sense that way given that it returns whether any errors were written itself.

I'm not sure what level of rigor you're looking for, but if you split this into a shard-handling layer and a package-handling layer you could property test this with proptest. The strategy (pun intended) would look something like this:

// inside test module

pub enum EntryType {
    File,
    Dir,
}

pub pkg_by_name_entry() -> impl Strategy<Value = (EntryType, String)> {
    (prop_oneof![
        EntryType::Dir,
        EntryType::File,
    ],
    <strategy generating strings>
    )
}

pub fn pkgs_by_name_entries(max_len: usize) -> impl Strategy<Value = Vec<(EntryType, String)>> {
    proptest::vec(pkgs_by_name_entry(), 0..max_len)
}

pub fn create_entries(base_dir: PathBuf, entries: Vec<(EntryType, String)>) {
    for (etype, name) in entries.iter() {
        let path = base_dir.join(name);
        use EntryType::*;
        match etype {
            Dir => <mkdir path>,
            File => <touch path>,
        }
    }
}

proptest!  {
    #[test]
    fn validates_shards(entries in pkgs_by_name_entries(100)) {
        let temp_dir = ...;
        create_entries(temp_dir, entries);
        # do some testing
        # delete temp_dir
    }
}

Thanks for the suggestion! I think this is a bit overboard, I'm fairly happy with the existing integration tests in ./tests, which do at least cover all of the potential errors. And the checking isn't so complicated that I think we can benefit a lot from property tests. Especially since it seems pretty hard to come up with beneficial ones. So I'd like to leave it at that for now.

Another point on this writer. If you wanted to make things more pure, you could separate out the various operations that could write error messages, then pass in a separate writer (doesn't need to be anything more complicated than a Vec) to each one. After each operation you would std::io::copy from the Vec to the main writer that actually does the printing.

You're obviously doing more I/O this way, but the tradeoff is that you don't have to pass around the same stateful object (writer) through the entire program and you could then write unit tests ensuring that error messages actually get written for various error conditions (e.g. in a test you would supply an empty Vec to the operation under test, run the operation, then check that writer.empty == false).

I'm not sure what you mean here. All of the functions already take a writer as an argument already, and I do pass a Vec in the tests to make sure each individual problem gives the expected error. I guess the tests could be for individual checks for better unit-testing, though I don't think it's very necessary here, since it's not a library meant to be re-used. Only the resulting CLI that will be used, so having the integration tests in ./tests seem good enough to me.

I don't think the package should be hidden, and it should be named as a proper package in snake case. Could you add it to all-packages.nix or is that planned for PR 1b?

It's not strictly hidden, you can build it using nix-build -A tests.nixpkgsCheckByName. But it's not meant to be installed by users or to be stable, so I don't think it should be in the main package set. In particular also, tests is not being recursed into by nix search/nix-env/search.nixos.org (tests.recurseForDerivations is not set), so it doesn't show up in searches.

Changing the attribute to nixpkgs-check-by-name though sounds okay (now done).

Thanks, applied!