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

Don't ask confirmation when pinning a new package #6309

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kit-ty-kate
Copy link
Member

Fixes #3199

While reading #3199 i asked myself why this question was asked and i couldn't find any reason. It was added in #1335 with no reasoning behind it either.

In my opinion the question is redundant as the user is already asking to pin the package(s).

master_changes.md Outdated Show resolved Hide resolved
@rjbou
Copy link
Collaborator

rjbou commented Nov 25, 2024

Discussion on dev meeting: We agree that the question is too much, but the information remains relevant as it may indicate that a repository is missing or that the package name is mistyped.
It worth adding a note about the unknown packages than remove completely the question.

(OpamConsole.colorise `bold "NEW"))
then raise Aborted;
if not (OpamPackage.has_name st.packages name) then
OpamConsole.note "Package %s was unknown until now"
Copy link
Member Author

Choose a reason for hiding this comment

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

If anyone has a better idea feel free to change it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion

Suggested change
OpamConsole.note "Package %s was unknown until now"
OpamConsole.note "Package %s does not exist in registered repositories."

Copy link
Member Author

Choose a reason for hiding this comment

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

as a user i would find this message confusing. The repositories we refer to here are opam repositories but they might understand source repositories instead since the command does something with a source repository usually.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
OpamConsole.note "Package %s was unknown until now"
OpamConsole.note "Package %s does not exist in opam repositories registered in the current switch."

?

Copy link
Member Author

Choose a reason for hiding this comment

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

why not

Copy link
Member Author

@kit-ty-kate kit-ty-kate Dec 17, 2024

Choose a reason for hiding this comment

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

actually i'm not sure. What do we want to say to users? Maybe something like:

Suggested change
OpamConsole.note "Package %s was unknown until now"
OpamConsole.note "%s is a new package."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

opam pin fails adding new or multiple packages when --no-action is present
2 participants