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 flag --unlock-transitive to pub upgrade #4403

Merged
merged 2 commits into from
Oct 7, 2024

Conversation

sigurdm
Copy link
Contributor

@sigurdm sigurdm commented Oct 3, 2024

pub upgrade x y z will only unlock x, y and z, but with the new flag pub upgrade --transitive x y z will unlock those packages and their transitive dependencies.

Same for --major-versions.

Fixes #3504

We could also consider making this the default behavior - it is most likely what people expect.

@sigurdm sigurdm requested review from jonasfj and szakarias October 3, 2024 14:38
@jonasfj
Copy link
Member

jonasfj commented Oct 3, 2024

This is a bit confusing, because:

  • pub upgrade and pub upgrade --transitive are the same thing.
  • pub upgrade foo and pub upgrade --transitive foo are NOT the same thing.

@sigurdm
Copy link
Contributor Author

sigurdm commented Oct 4, 2024

pub upgrade and pub upgrade --transitive are the same thing.

Well, pub upgrade in my mind means pub upgrade <all dependencies> and the transitive closure of <all dependendencies> is still <all dependencies>.

So it is the same rule we apply.

But we could disallow --transitive when there are no given deps to update.

@sigurdm
Copy link
Contributor Author

sigurdm commented Oct 4, 2024

Let's call it --unlock-transitive.

@sigurdm
Copy link
Contributor Author

sigurdm commented Oct 4, 2024

This will not work in cases where the latest version of the upgraded package has more dependencies than the current.

Then they will not be unlocked.

Say:

foo 1.0.0 has

dependencies:
  # no dependencies

foo 1.5.0 has

dependencies:
  bar: ^1.0.0

And the main pubspec.yaml looks like

dependencies:
  foo: ^1.0.0
  bar: ^1.0.0 # bar will not be unlocked by `pub upgrade foo --unlock-transitive`

We have agreed that is a relatively rare case, and we can live with it.

@sigurdm sigurdm changed the title Add flag --transitive to pub upgrade Add flag --unlock-transitive to pub upgrade Oct 4, 2024
Copy link
Member

@jonasfj jonasfj left a comment

Choose a reason for hiding this comment

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

Please add the negatable: false, if we want the negatable variant I think we should call it --lock-transitive rather than --nounlock-transitive 🤣

Comment on lines +73 to +77
argParser.addFlag(
'unlock-transitive',
help: 'Also upgrades the transitive dependencies '
'of the listed [dependencies]',
);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
argParser.addFlag(
'unlock-transitive',
help: 'Also upgrades the transitive dependencies '
'of the listed [dependencies]',
);
argParser.addFlag(
'unlock-transitive',
help: 'Also upgrades the transitive dependencies '
'of the listed [dependencies]',
negatable: false,
);

lib/src/command/upgrade.dart Show resolved Hide resolved
@@ -10,6 +10,7 @@ Usage: pub upgrade [dependencies...]
-n, --dry-run Report what dependencies would change but don't change any.
--[no-]precompile Precompile executables in immediate dependencies.
--tighten Updates lower bounds in pubspec.yaml to match the resolved version.
--[no-]transitive Also upgrades the transitive dependencies of the listed [dependencies]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
--[no-]transitive Also upgrades the transitive dependencies of the listed [dependencies]
--transitive Also upgrades the transitive dependencies of the listed [dependencies]

@sigurdm
Copy link
Contributor Author

sigurdm commented Oct 7, 2024

Please add the negatable: false, if we want the negatable variant I think we should call it --lock-transitive rather than --nounlock-transitive 🤣

I disagree. The negations are useful if we ever change the default. And in that case they should also be present from the beginning.

I think --no-unlock-transitive reads ok for a very rarely used option. (And we are specifically asking pub to not unlock the transitive dependencies. Not to lock them.

@sigurdm sigurdm merged commit a2026fa into dart-lang:master Oct 7, 2024
18 of 23 checks passed
@sigurdm
Copy link
Contributor Author

sigurdm commented Oct 7, 2024

Seems like I merged this by accident...

Will revert.

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.

pub upgrade some_package doesn't upgrade transitive dependencies
2 participants