Skip to content

Number every problem reported by nixpkgs-vet#109

Merged
philiptaron merged 40 commits intoNixOS:mainfrom
philiptaron:28-problems
Oct 5, 2024
Merged

Number every problem reported by nixpkgs-vet#109
philiptaron merged 40 commits intoNixOS:mainfrom
philiptaron:28-problems

Conversation

@philiptaron
Copy link
Copy Markdown
Contributor

Motivation for changes

Description of changes

  • Make a struct for each error reported by nixpkgs-vet
  • Have it Display with the exact same error as previously
  • Change call site to report the specific error rather than use a to_validation closure

This PR is best reviewed commit-by-commit.

This tool is called `nixpkgs-vet`. Implicitly, every thing it talks about is prefixed by "nixpkgs",
so it doesn't need the explicit prefix.
We have to do a little work to transition the callsite, but it all works.
It's more about structure than anything else, and it's used here primarily.
Let's automate it with the `derive-new` proc macro.
@willbush
Copy link
Copy Markdown
Member

I'll get a chance to look at this closer tomorrow or this weekend

@philiptaron
Copy link
Copy Markdown
Contributor Author

Gentle ping to review if you've got the cycles to spend.

@willbush
Copy link
Copy Markdown
Member

willbush commented Oct 5, 2024

@philiptaron Sorry for the wait! Seems like good changes all around to me. More widespread usage of Into trait and additional crates make a lot of sense. Really appreciate you breaking this commits up (looks like a lot of work). Only nit for future reference fn read_dir_sorted got moved and changed in the same commit; it's easy to miss that it was changed at all. :)

@philiptaron philiptaron merged commit 54a8777 into NixOS:main Oct 5, 2024
@philiptaron philiptaron deleted the 28-problems branch October 7, 2024 14:12
@infinisil
Copy link
Copy Markdown
Member

Nice work @philiptaron, and thanks for reviewing @willbush! Sorry that I'm not very available recently 😅

@mdaniels5757 mdaniels5757 mentioned this pull request Mar 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants