Skip to content

dependency_services #3304

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

Merged
merged 44 commits into from
Feb 10, 2022
Merged

dependency_services #3304

merged 44 commits into from
Feb 10, 2022

Conversation

sigurdm
Copy link
Contributor

@sigurdm sigurdm commented Feb 4, 2022

This will provide support for dependabot resolution.

See dependabot/dependabot-core#4510 for usage.

This will not be exposed in the sdk.

@sigurdm sigurdm changed the title merge with mainline dependency_services Feb 4, 2022
@sigurdm sigurdm requested a review from jonasfj February 4, 2022 13:26
@@ -288,7 +288,7 @@ class SolveReport {
// Only show newer prereleases for versions where a prerelease is
// already chosen.
newId.version.isPreRelease && newerUnstable) {
message = '(${maxAll(versions)} available)';
message = '(${maxAll(versions, Comparable.compare)} available)';
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we do. Not sure why this sneaked in.

Comment on lines 121 to 124
await listReportApply(context, [
_PackageVersion('foo', Version.parse('2.2.3')),
_PackageVersion('transitive', null)
]);
Copy link
Member

Choose a reason for hiding this comment

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

As much as I love golden tests, maybe we should add some assertions here?

There is a non-zero risk that we won't notice minor changes in the golden files.

If we could do a simple assertion like:

expect(report['dependencies'].any((depReport) => depReport['name'] == 'foo' && depReport['singleBreaking'].any(
  (d) => d['name'] == 'transitive' && d['version'] == null),
), isTrue);

Or something like that... just a few, we don't have to check every possible property :D

-------------------------------- END OF OUTPUT ---------------------------------

## Section apply
$ echo '{"dependencyChanges":[{"name":"foo","version":"3.0.1","constraint":"^3.0.0"},{"name":"bar","version":"2.0.0"}]}' | dependency_services apply
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't there also be a constraint for bar? Just asking... I'm not actually sure we require that... probably because dependabot loses this information...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so. This is a single-breaking change. Only foo should have its constraint updated.

Also depandabot would lose this information

Copy link
Member

Choose a reason for hiding this comment

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

I think this is the multibreaking example.... but yes dependabot will probably loose this information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah you're right.

@@ -17,7 +17,7 @@ Changed 1 dependency in myapp!
-------------------------------- END OF OUTPUT ---------------------------------

## Section 2
$ pub -C myapp/example get --directory=myapp bar
$ pub -C 'myapp/example' get --directory=myapp bar
Copy link
Member

Choose a reason for hiding this comment

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

Should we do a tweak to escaping mechanism, so that we don't need to escape when there is a / or . in the string... those are generally safe in bash, right? (and quite common in our tests)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah we should.

The issue with / is a technical annoyance. We have test filter that turns '' into '/' in the golden tests on windows. But the escaping happens before...

@sigurdm sigurdm merged commit 941191f into dart-lang:master Feb 10, 2022
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.

2 participants