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

Tweak wording to make "no changes" message more accurate #3806

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ericphanson
Copy link
Contributor

@ericphanson ericphanson commented Feb 23, 2024

fixes #3066

(cc @haberdashPI)

edit: I'll need to update the tests

@IanButterworth
Copy link
Sponsor Member

No packages added to or removed from ..

Changes could be a lot more things than just added or removed i.e. version changes, path changes, pin status etc.

I had proposed

No changes to the dependencies in /path/to/Manifest.toml but metadata was updated

If we don't want to mention the metadata part because it's hard to track and know whether it's changed could just say

No changes to the dependencies in /path/to/Manifest.toml

@haberdashPI
Copy link

haberdashPI commented Feb 27, 2024

No changes to the dependencies in /path/to/Manifest.toml

This is quite misleading as a user with no deep knowledge of Pkg internals (and the metadata part only adds a bit to my confusion). I think it has the same problems as the extant message.

I think the main problem with the current wording is that as a user "change" can be understood to mean any change to the file on disk regarding a dependency. But e.g. resolve might change whether a dev'ed package can be compiled successfully, which clearly means there was a change to one of the dependencies in Manifest.toml (right??), even though this message will show up in that case.

What about:

No dependencies needed to be installed or uninstalled due to the changes in Project.toml/Manifest.toml

This makes it clear that there were indeed changes to the toml files.

@ericphanson
Copy link
Contributor Author

ericphanson commented Feb 28, 2024

replying to @IanButterworth:

Changes could be a lot more things than just added or removed i.e. version changes, path changes, pin status etc.

So this message prints when all(p-> p[2] == p[3], xs) holds, which is some statement about the difference in env caches. I think you are saying roughly: "if there is no difference in env caches, we know more than no packages were added or removed! we also know you didn't pin something, or change a version, or change a path!" And therefore the message should reflect this stronger notion of lack of change.

My counter is: the current message is basically wrong (there are, in fact, changes to the manifest/project in many cases). The new message I propose here is actually 100% correct, it is just not as strong as it possibly could be; we could be making even bigger claims about not only packages not changing but also pin status not changing etc.

So the question is, how much does making bigger claims here help the user? Well, Pkg helpfully prints most changes/updates/etc, so this message is mostly in lieu of printing nothing, so the user knows the command actually ran. So what we need to tell the user is: "we ran your command and there's nothing worth reporting about specifically". I don't think that needs the strongest possible wording. I think that just shouldn't overstate the case.

Therefore, in my opinion, the current simple wording I propose is an improvement over the status quo (saying false things -> saying true things), and that a more nuanced printing about exactly how little has changed isn't super useful.

No changes to the dependencies in /path/to/Manifest.toml

In the example in #3066, I think in some sense a "change" was indeed made to a dependency in the manifest. It was resolved and its dependencies updated. Part of the reason #3066 is very annoying is that Julia says "your manifest is wrong! fix it!" and then you do so and it says "no changes were made to the manifest". But secretly, a change was made, and it fixed the issue it was complaining about.

I think largely the same issue holds with this proposed wording. Pkg saying "No changes to the dependencies in /path/to/Manifest.toml" is slightly better than the status quo (at least it is a qualified statement) but it still seems like a change was made to the dependencies that fixed the issue Julia was complaining about (I had an issue w/ my deps -> I don't have an issue with my deps), and therefore it's misleading to claim no change was made to the deps.


replying to @haberdashPI:

No dependencies needed to be installed or uninstalled due to the changes in Project.toml/Manifest.toml

In the code where this change is, we don't know whether or not there were changes to the Project/Manifest IIUC.

@haberdashPI
Copy link

I'm good with your wording @ericphanson, but I could also imagine:

No dependencies were installed or uninstalled

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.

"No Changes to ~/PkgA/Manifest.toml" is misleading since there can be changes
3 participants