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

pub upgrade command to show number of discontinued packages #2908

Merged
merged 3 commits into from
Mar 12, 2021

Conversation

tusharojha
Copy link
Contributor

Description

I have added a message showing the number of discontinued packages for pub upgrade command.

Fixes #2865

@google-cla google-cla bot added the cla: yes label Mar 10, 2021
@tusharojha
Copy link
Contributor Author

@sigurdm PTAL.

Copy link
Contributor

@sigurdm sigurdm left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for the contribution!

We need a test to test this behavior.
See https://github.com/dart-lang/pub/blob/5aadb70e0a59017968a4898b383798d7d507152e/test/get/hosted/warn_about_discontinued_test.dart for inspiration on how to set up a discontinued package in the test framework.

/// Displays a single-line message, number of discontinued packages
/// if discontinued packages are detected.
Future<void> reportDiscontinued() async {
var numDiscontinued = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit: follow the style from reportOutdated.

final discontinuedCount = result.packages.where((pkg) => ...).length;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I tried to do it using where but the method needs to async which is not possible with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah! - good point. Did not think of that. Then your solution is fine.

lib/src/solver/result.dart Show resolved Hide resolved
@tusharojha
Copy link
Contributor Author

@sigurdm I have added a test case. PTAL.

@@ -50,6 +50,50 @@ void main() {
]).validate();
});

test('--dry-run: shows count of discontinued packages', () async {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
test('--dry-run: shows count of discontinued packages', () async {
test('Shows count of discontinued packages', () async {

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this test belongs better in for example test/upgrade/report/describes_change_test.dart . It does not really relate to dry-run.

d.file('pubspec.lock', contains('2.0.0')),
// The ".dart_tool" directory should not have been regenerated.
d.nothing('.dart_tool')
]).validate();
Copy link
Contributor

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 need this validation in this test. We want to test that we print the count, not how --dry-run behaves.

Comment on lines 156 to 160
if (numDiscontinued > 1) {
log.message('$numDiscontinued packages are discontinued.');
} else if (numDiscontinued == 1) {
log.message('1 package is discontinued.');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would be slightly easier to follow:

Suggested change
if (numDiscontinued > 1) {
log.message('$numDiscontinued packages are discontinued.');
} else if (numDiscontinued == 1) {
log.message('1 package is discontinued.');
}
if (numDiscontinued > 0) {
if (numDiscontinued == 1) {
log.message('1 package is discontinued.');
} else {
log.message('$numDiscontinued packages are discontinued.');
}
}

@tusharojha
Copy link
Contributor Author

@sigurdm Thanks for the review!
I have updated the code as per your suggestions.

Copy link
Contributor

@sigurdm sigurdm left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks!

@sigurdm sigurdm merged commit d6308ef into dart-lang:master Mar 12, 2021
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.

Upgrade summary should show a count of discontinued packages
2 participants