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 opam pin remove --all #5308

Merged
merged 2 commits into from
Nov 16, 2022
Merged

Conversation

kit-ty-kate
Copy link
Member

I often find the need to clean all pins from the current switch i am working on, be it because the pin packages have all been released or other reasons, and listing all the packages or repos one by one can be extremely tedious.

@avsm
Copy link
Member

avsm commented Oct 7, 2022

Yes please! I also have the case that it's difficult to undo an opam install's pins

opam install .
# proceeds to pin all the local packages
# how do i undo the previous command? opam remove doesn't unpin them.

@kit-ty-kate
Copy link
Member Author

Could you open a new issue to track that problem?

src/client/opamCommands.ml Outdated Show resolved Hide resolved
src/client/opamCommands.ml Outdated Show resolved Hide resolved
src/client/opamCommands.ml Outdated Show resolved Hide resolved
tests/reftests/pin.test Show resolved Hide resolved
tests/reftests/pin.test Outdated Show resolved Hide resolved
src/core/opamStd.mli Show resolved Hide resolved
@kit-ty-kate
Copy link
Member Author

All done

Copy link
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

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

Tiny tweak to the message.

More importantly, -a has a lot of different meanings across opam already, so this doesn't technically make things that much worse, however opam remove -a removes automatically-installed packages but opam pin remove -a would now mean remove all pins. Might it be worth either using a different letter (opam admin already has -A for a --all option) or just not having a short form at all for this one?

src/client/opamCommands.ml Outdated Show resolved Hide resolved
src/client/opamCommands.ml Outdated Show resolved Hide resolved
@avsm
Copy link
Member

avsm commented Oct 17, 2022

I'm not convinced we need this PR because:

  1. opam pin remove -a has a different meaning from opam remove -a (as @dra27 notes)
  2. opam pin remove $(opam pin -s) accomplishes what this PR does -- we could document this.
  3. opam pin install and opam pin remove are not currently symmetric, and there's no easy way to do this.

I'd recommend not adding more CLI options to opam when there are existing unix-style solutions that work, and focussing on fixing the really rough edges like 3) above which is hard to do (as you cannot easily probe the set of locally pinned packages without some wildcard magic)

@kit-ty-kate kit-ty-kate changed the title Add opam pin remove --all/-a Add opam pin remove --all Oct 17, 2022
@kit-ty-kate kit-ty-kate added the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Oct 21, 2022
@kit-ty-kate kit-ty-kate force-pushed the pin-remove-all branch 2 times, most recently from 15beb46 to cab258f Compare November 15, 2022 13:37
@kit-ty-kate kit-ty-kate added PR: WAITING FOR REVIEW and removed PR: QUEUED Pending pull request, waiting for other work to be merged or closed labels Nov 15, 2022
tests/reftests/pin.test Outdated Show resolved Hide resolved
tests/reftests/pin.test Show resolved Hide resolved
@@ -3184,6 +3184,10 @@ let pin ?(unpin_only=false) cli =
changes, and may also be used to keep a package that was removed \
upstream."
in
let all =
mk_flag ~cli (cli_from cli2_2) ["all"]
"When unpinning, removes all pins in the given switch."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add a word about this command being a no-op on other pin commands ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I chose to make the use of --all outside of remove without any parameter an error instead. Does that work for you?

Copy link
Collaborator

Choose a reason for hiding this comment

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

good for me!

@rjbou rjbou added this to the 2.2.0~alpha milestone Nov 16, 2022
@kit-ty-kate kit-ty-kate merged commit b540b2e into ocaml:master Nov 16, 2022
@kit-ty-kate kit-ty-kate deleted the pin-remove-all branch November 16, 2022 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants