-
Notifications
You must be signed in to change notification settings - Fork 105
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
Update subcommand #140
Update subcommand #140
Conversation
This will allow us to test the update tool in all expected situations - One dependency added/removed - One dependency updated - Transitive dependencies changed It's also a nice addition for testing any other commands as well.
Metadata and zipped up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great start! To formalise a discussion from discord:
This should modify the manifest as well as the lockfile. We should also notify the user if there is a newer semver incompatible version of a package which we can't auto upgrade to. The thought process behind this is people will usually wally update
blind and may be unaware there is a new major version of a package.
We should also add convenience flags to allow the user to specify if they want to upgrade everything to the latest possible version even if they are semver incompatible or if they do not want any semver incompatible choices. They might have many dependencies and saying yes or no is annoying!
It will generate a new one instead based on the manifest.
-- they got stashed by accident
They're not packages to be consumed.
We should also add convenience flags to allow the user to specify if they want to upgrade everything to the latest possible version even if they are semver incompatible or if they do not want any semver incompatible choices. They might have many dependencies and saying yes or no is annoying! This should be postponed and done in a separate PR. It turns it, trying to do this gets complicated really fast.
I think it's better to keep the velocity going then to get this stuck here. |
also the test cases are still broken :( |
They say to run your tests before push.. This is why.
Import in the wrong order... Extra newline at the end...
a457ae5
to
839c713
Compare
Ah yeah, I agree then. It is far more important that the command exists than for it to be ergonomic in all cases. If anyone really does want to upgrade everything to latest they can just yeet their lockfile and reinstall anyway. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great! I believe it now works as expected and is tested enough to trust! I just have a couple nitpicks for you to check.
tests/integration/snapshots/integration__update__update_all_dependencies-2.snap
Outdated
Show resolved
Hide resolved
Co-authored-by: magnalite <[email protected]>
Seemingly forgot to filter out packages to update for try_to_use. instead it kept them and got rid of everything else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work getting this polished up!
* Refactored TempProject into its own module and started on tests * Created test projects with a diamond-graph This will allow us to test the update tool in all expected situations - One dependency added/removed - One dependency updated - Transitive dependencies changed It's also a nice addition for testing any other commands as well. * Added to primary-registry Metadata and zipped up * Built the update subcommand. * Lockfiles need to be written! * Fixed misspelling of dependency_changes * Reverted test_registry arg back to invisible * Changed target_packages to package_specs Clarity? * Downgrading was added as a concept * Added lockfile method to view packages as packageIds * Installs the new packages now, forgor. * No longer throws if missing lockfile It will generate a new one instead based on the manifest. * Clarity on the filtering process for try_to_use * Added needed imports -- they got stashed by accident * Made the clippy happy and fixed grammar mistakes * Delete root from registry They're not packages to be consumed. * Add new tests, almost done with them all so far. * Added the rest of snapshots missing * Testing now works, yay! * Deleted testing code by mistake and forgot snap They say to run your tests before push.. This is why. * Appleasing the formatter Import in the wrong order... Extra newline at the end... * Made it look... *pretty* and cleaned messes * another blood sarciface for rust fmt * Doing final cleanups per comments upstream * The gods demand it, another formatter sacrifice Co-authored-by: magnalite <[email protected]> * The coolness must be toned down. * A little silly mistake indeed Seemingly forgot to filter out packages to update for try_to_use. instead it kept them and got rid of everything else. --------- Co-authored-by: magnalite <[email protected]>
This adds an update subcommand per issue #4.