Repository navigation
Streamline package shortcuts for add and updates - #441
Merged
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors niv add / niv update CLI parsing to remove per-command “add” subcommands (git/local/github) and instead rely on a unified shortcut-expansion + attribute parsing flow, updating tests and docs accordingly.
Changes:
- Reworks
addto accept a singlePACKAGEplus optional--nameand groupedATTRIBUTES, and expands shortcuts by trying all registeredCmds. - Reworks
updateto optionally target a single package name and optionally apply attribute overrides, plus adds duplicate-attribute validation. - Updates README and Nix tests to match the new CLI interface; bumps/overrides
optparse-applicativeto 0.19+.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/local/default.nix | Updates local add test to use unified niv add … [ATTRIBUTES] interface. |
| tests/git/default.nix | Updates git add test to use unified niv add … [ATTRIBUTES] interface. |
| src/Niv/Sources.hs | Adds Eq deriving to PackageSpec. |
| src/Niv/Local/Cmd.hs | Removes per-command spec parsing; updates shortcut parsing to return PackageSpec. |
| src/Niv/GitHub/Cmd.hs | Removes per-command spec parsing; updates shortcut parsing to return PackageSpec. |
| src/Niv/Git/Test.hs | Updates tests to expect PackageSpec instead of raw Aeson.Object. |
| src/Niv/Git/Cmd.hs | Removes per-command spec parsing; updates shortcut parsing to return PackageSpec. |
| src/Niv/Cmd.hs | Simplifies Cmd interface (drops description/parsePackageSpec; shortcut returns PackageSpec). |
| src/Niv/Cli.hs | Implements unified add/update argument parsing, shortcut expansion, and duplicate-attribute validation. |
| script/test | Adds --max-jobs auto to nix build invocation. |
| README.md | Updates CLI docs/examples for new add/update/modify usage format. |
| niv.cabal | Requires optparse-applicative >= 0.19.0.0. |
| flake.nix | Overrides Haskell package set to provide optparse-applicative 0.19.0.0 for builds. |
Suppressed comments (6)
src/Niv/Cli.hs:505
foldl'is used here but isn’t imported anywhere in this module, which will fail compilation. You can avoid the extra import by building the counts map withHashMap.fromListWithinstead.
foldl'
(\acc (k, _) -> HMS.alter (\case Nothing -> Just (1 :: Int); Just n -> Just (n + 1)) k acc)
HMS.empty
parsed
offending = HMS.filter (\n -> n > 1) counts
src/Niv/Cli.hs:895
- The comment above
abortManyCommandsForShortcutis incorrect: this abort is for an ambiguous shortcut (matched multiple commands), not for “no update Cmd is suited to the package”.
-- Error if no update Cmd is suited to the package
src/Niv/Cli.hs:570
- The
--typehelp text currently describes only the url-template fetch "type" (file/tarball), but this option is also used to set the package updater type (e.g.--type git,--type localas in the tests). The help text should mention both to avoid misleading CLI docs.
"type"
( Opts.long "type"
<> Opts.short 'T'
<> Opts.metavar "TYPE"
<> Opts.help "The type of the URL target. The value can be either 'file' or 'tarball'. If not set, the value is inferred from the suffix of the URL."
src/Niv/Cli.hs:393
- This branch is redundant: after
HMS.lookup (PackageName pat)fails, filtering byunPackageName == patcan never succeed (same equality). Also the surrounding comments call this a “pattern”, but the implementation is exact-match only—better to simplify/clarify to avoid misleading behavior.
-- pattern (filter) provided: match exact
Just (PackagePattern pat) -> case HMS.lookup (PackageName pat) sources of
Just exact -> HMS.singleton (PackageName pat) exact
Nothing ->
HMS.filterWithKey (\k _ -> unPackageName k == pat) sources
README.md:298
- Same as in the Add section:
-T/--typeis documented as only file/tarball here, but it’s also used to select the package updater type (git/local). Update this help text to reflect both uses.
-T,--type TYPE The type of the URL target. The value can be either
'file' or 'tarball'. If not set, the value is
inferred from the suffix of the URL.
README.md:345
- Same doc issue as above:
-T/--typeis described only as file/tarball, but this PR uses--type git/--type localfor package selection. Please clarify the meaning here as well.
-T,--type TYPE The type of the URL target. The value can be either
'file' or 'tarball'. If not set, the value is
inferred from the suffix of the URL.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.