Pub dependencies project validator #106895

Jasguerrero · 2022-06-30T19:14:57Z

Pre-launch Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
I signed the [CLA].
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is [test-exempt].
All existing and new tests are passing.

christopherfujino · 2022-06-30T19:39:27Z

packages/flutter_tools/test/general.shard/pub_dependencies_project_validator_test.dart

+  group('PubDependenciesProjectValidator', () {
+
+    setUp(() {
+      fileSystem = globals.localFileSystem;


does this need to use the real file system? and if so, this should probably be in the integration shard.

Not really. I can use a FakeFileSystem here

christopherfujino · 2022-06-30T19:40:03Z

packages/flutter_tools/test/general.shard/pub_dependencies_project_validator_test.dart

+      fileSystem = globals.localFileSystem;
+    });
+
+    testWithoutContext('success ', () async {


out of curiosity, why the trailing space in the test name?

That was a typo

christopherfujino · 2022-06-30T19:40:59Z

packages/flutter_tools/test/general.shard/pub_dependencies_project_validator_test.dart

+      fileSystem = globals.localFileSystem;
+    });
+
+    testWithoutContext('success ', () async {


also, maybe name this 'success when all dependencies are hosted'

christopherfujino · 2022-06-30T19:41:26Z

packages/flutter_tools/test/general.shard/pub_dependencies_project_validator_test.dart

+      final ProcessManager processManager = FakeProcessManager.list(<FakeCommand>[
+        const FakeCommand(
+          command: <String>['dart', 'pub', 'deps', '--json'],
+          stdout: 'stdout command fail',


what does this mean?

Just expecting something not a json (eg: if you haven't updated your pub dependencies and the pubspec has changed pub dart deps returns a message to update them. I don't have the exact message so I just fake this)

Also if for some reason dart pub deps --json breaks we will catch that and pass the stacktrace to the validator output

Good thinking. However, if the command breaks any debug messaging should go to STDERR, right?

I could be wrong, but I would imagine if you pass --json, only valid JSON should be output to stdout, and any diagnostics would go to stderr.

christopherfujino · 2022-06-30T19:42:09Z

packages/flutter_tools/test/general.shard/pub_dependencies_project_validator_test.dart

+      final List<ProjectValidatorResult> result = await validator.start(
+          FlutterProject.fromDirectoryTest(fileSystem.currentDirectory)
+      );
+      const String expected = 'All dependencies are hosted';


will the user have enough context to understand what this means?

The user will read [✓] Dart dependencies: All dependencies are hosted

will they know what that means?

I don't know. Should I add something like [✓] Dart dependencies: All dart dependencies comes from known places or something along those lines?

will they know what "comes from" means?

I guess backing up, what is the high level information that you want to communicate to the user?

If one ore more dependencies do not come from pub.dev eg: they are pointing to a github repo to get it #103614

christopherfujino · 2022-06-30T19:43:02Z

packages/flutter_tools/test/general.shard/pub_dependencies_project_validator_test.dart

+      final List<ProjectValidatorResult> result = await validator.start(
+          FlutterProject.fromDirectoryTest(fileSystem.currentDirectory)
+      );
+      const String expected = 'dep1, dep2 are not hosted';


same question, will the user have enough context to understand what this means? in other words, are we going to get bug reports that they're getting warnings on their project when everything works?

christopherfujino · 2022-06-30T19:44:26Z

packages/flutter_tools/lib/src/project_validator.dart

+
+    final DartPubJson dartPubJson = DartPubJson(jsonResult);
+    final List <String> dependencies = <String>[];
+    final Set<String> hostedDependencies = {'hosted', 'root'};


what does 'root' mean? please add a comment explaining both hosted and root.

christopherfujino · 2022-06-30T19:46:04Z

packages/flutter_tools/lib/src/project_validator.dart

 }
+
+class PubDependenciesProjectValidator extends ProjectValidator {
+  PubDependenciesProjectValidator(this._processManager);


Suggested change

PubDependenciesProjectValidator(this._processManager);

const PubDependenciesProjectValidator(this._processManager);

I couldn't implement this because of the abstract class

Can you make the super also const?

you'll have to make title final, but i don't think that should be a problem

christopherfujino · 2022-06-30T19:55:27Z

packages/flutter_tools/lib/src/dart_pub_json_formatter.dart

@@ -0,0 +1,62 @@
+import 'dart:collection';
+
+class Package {


this definitely needs a dartdoc, and maybe a more specific name. Note that since the convention in Dart is to just do import 'foo.dart';, every name gets imported into that library's namespace

christopherfujino · 2022-06-30T19:57:26Z

packages/flutter_tools/lib/src/commands/analyze.dart

-        if (items.isEmpty || items.length > 1) {
-          throwToolExit('The suggestions flags needs one directory path');
+        if (items.isEmpty) { // user did not specify any path
+          _logger.printStatus('Showing suggestions for current directory');


Suggested change

_logger.printStatus('Showing suggestions for current directory');

_logger.printTrace('Showing suggestions for current directory: ${_fileSystem.currentDirectory.path}');

christopherfujino · 2022-07-07T21:02:46Z

packages/flutter_tools/lib/src/dart_pub_json_formatter.dart

  final List<String> dependencies;

-  static Package fromHashMap(dynamic packageInfo) {
+  static DartDependencyPackage fromHashMap(dynamic packageInfo) {


Dart style nit:

Suggested change

static DartDependencyPackage fromHashMap(dynamic packageInfo) {

factory DartDependencyPackage.fromHashMap(dynamic packageInfo) {

this is a style nit, this could very well compile to the same machine code

christopherfujino · 2022-07-14T21:13:20Z

packages/flutter_tools/test/integration.shard/analyze_suggestions_integration_test.dart

+      const String expected = '\n'
+        '┌────────────────────────────────────────────────────────────────────────────────────┐\n'
+        '│ Pub dependencies                                                                   │\n'
+        '│ [✓] Dart dependencies: All pub dependencies are hosted in https://pub.dartlang.org │\n'


Suggested change

'│ [✓] Dart dependencies: All pub dependencies are hosted in https://pub.dartlang.org │\n'

'│ [✓] Dart dependencies: All pub dependencies are hosted on https://pub.dartlang.org │\n'

christopherfujino

LGTM

christopherfujino · 2022-07-14T21:18:55Z

nit: let's just make these all green, success, since it's not really a problem. Follow-up will be in #107675

Jasguerrero added 3 commits June 29, 2022 16:15

add dart pub json formatter

b717a3b

add test for pub dependencies validator

055d5cd

move comment for project validators to execute

fc7ee28

Jasguerrero requested a review from christopherfujino June 30, 2022 19:14

flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jun 30, 2022

christopherfujino reviewed Jun 30, 2022

View reviewed changes

Jasguerrero added 2 commits July 7, 2022 10:49

update test with memory fs and rename package cls

bbc8607

update output of suggestions dependencies

645cd58

christopherfujino reviewed Jul 7, 2022

View reviewed changes

Jasguerrero added 3 commits July 7, 2022 14:17

use factory instead of static method

9aac74b

specify type anotations

260d422

const constructor

9c74dde

christopherfujino reviewed Jul 14, 2022

View reviewed changes

christopherfujino approved these changes Jul 14, 2022

View reviewed changes

Jasguerrero added 2 commits July 15, 2022 12:27

Merge branch 'master' into pub_dependencies_project_validator

ec95c3a

update log

e7e8f60

Jasguerrero added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 15, 2022

auto-submit bot merged commit cdf5b1d into flutter:master Jul 15, 2022

This was referenced Jul 16, 2022

Roll Flutter from c4975d9be0e2 to 4f9528293f54 (16 revisions) flutter/plugins#6111

Merged

Roll Flutter from bbec650b64bc to 4f9528293f54 (21 revisions) flutter/packages#2327

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 21, 2022

cdf5b1d Pub dependencies project validator (flutter/flutter#106895)

0612e29

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 22, 2022

cdf5b1d Pub dependencies project validator (flutter/flutter#106895)

a06d6d1

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 22, 2022

cdf5b1d Pub dependencies project validator (flutter/flutter#106895)

8744f10

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 22, 2022

cdf5b1d Pub dependencies project validator (flutter/flutter#106895)

c664eee

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 22, 2022

cdf5b1d Pub dependencies project validator (flutter/flutter#106895)

dfc3069

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 22, 2022

cdf5b1d Pub dependencies project validator (flutter/flutter#106895)

0a29f83

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 23, 2022

cdf5b1d Pub dependencies project validator (flutter/flutter#106895)

6e39736

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 23, 2022

cdf5b1d Pub dependencies project validator (flutter/flutter#106895)

f07c80b

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 24, 2022

cdf5b1d Pub dependencies project validator (flutter/flutter#106895)

89162b3

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 24, 2022

cdf5b1d Pub dependencies project validator (flutter/flutter#106895)

7bf12a1

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 25, 2022

cdf5b1d Pub dependencies project validator (flutter/flutter#106895)

d57d7af

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 25, 2022

cdf5b1d Pub dependencies project validator (flutter/flutter#106895)

f038e8d

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 25, 2022

cdf5b1d Pub dependencies project validator (flutter/flutter#106895)

c72a9b0

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 25, 2022

cdf5b1d Pub dependencies project validator (flutter/flutter#106895)

d6b7848

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 25, 2022

cdf5b1d Pub dependencies project validator (flutter/flutter#106895)

96d35de

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 25, 2022

cdf5b1d Pub dependencies project validator (flutter/flutter#106895)

488026e

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 25, 2022

cdf5b1d Pub dependencies project validator (flutter/flutter#106895)

6ba381e

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 25, 2022

cdf5b1d Pub dependencies project validator (flutter/flutter#106895)

ef95d1c

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 25, 2022

cdf5b1d Pub dependencies project validator (flutter/flutter#106895)

a58b055

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 25, 2022

cdf5b1d Pub dependencies project validator (flutter/flutter#106895)

1f59430

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 25, 2022

cdf5b1d Pub dependencies project validator (flutter/flutter#106895)

b6c5c0f

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 25, 2022

cdf5b1d Pub dependencies project validator (flutter/flutter#106895)

67e9ff1

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 26, 2022

cdf5b1d Pub dependencies project validator (flutter/flutter#106895)

01b5bd7

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 26, 2022

cdf5b1d Pub dependencies project validator (flutter/flutter#106895)

e206541

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 26, 2022

cdf5b1d Pub dependencies project validator (flutter/flutter#106895)

75c8089

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 26, 2022

cdf5b1d Pub dependencies project validator (flutter/flutter#106895)

f7c072c

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 26, 2022

cdf5b1d Pub dependencies project validator (flutter/flutter#106895)

b1b9c03

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 26, 2022

cdf5b1d Pub dependencies project validator (flutter/flutter#106895)

dbd6948

camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022

Pub dependencies project validator (flutter#106895)

0e20b43

	PubDependenciesProjectValidator(this._processManager);
	const PubDependenciesProjectValidator(this._processManager);

	_logger.printStatus('Showing suggestions for current directory');
	_logger.printTrace('Showing suggestions for current directory: ${_fileSystem.currentDirectory.path}');

	static DartDependencyPackage fromHashMap(dynamic packageInfo) {
	factory DartDependencyPackage.fromHashMap(dynamic packageInfo) {

	'│ [✓] Dart dependencies: All pub dependencies are hosted in https://pub.dartlang.org │\n'
	'│ [✓] Dart dependencies: All pub dependencies are hosted on https://pub.dartlang.org │\n'

Pub dependencies project validator #106895

Pub dependencies project validator #106895

Uh oh!

Conversation

Jasguerrero commented Jun 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jasguerrero Jun 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jasguerrero Jun 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

christopherfujino commented Jul 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Jasguerrero commented Jun 30, 2022 •

edited

Loading

Jasguerrero Jun 30, 2022 •

edited

Loading

Jasguerrero Jun 30, 2022 •

edited

Loading