v1.2.0 - findAll, ImportPath class and use of ascii_art_tree. #3

gmpassos · 2023-09-03T21:20:44Z

Moved code to class ImportPath:
- Allows integration with other packages.
- Facilitates tests.
Using ascii_art_tree to show the output tree, with styles dots (original) and elegant.
CLI:
- Added options:
  - --regexp: to use RegExp to match the target import.
  - --all: to find all the import paths.
  - -q: for a quiet output (only displays found paths).
  - -s: strips the search root directory from displayed import paths.
  - --elegant: use elegant style for the output tree.
  - --dots: use dots style for the output tree.
- Improved help with examples.
Updated README.md to show CLI and Library usage.
Added tests and coverage (80%).
Added GitHub Dart CI (linux, windows, macOS).
Updated dependencies compatible with Dart 2.14.0 (was already dependent to SDK 2.14.0):
- sdk: '>=2.14.0 <3.0.0'
- package_config: ^2.1.0
- path: ^1.8.3
- ascii_art_tree: ^1.0.4
- lints: ^1.0.1
- dependency_validator: ^3.2.2
- test: ^1.21.4
- coverage: ^1.2.0

- Moved code to class `ImportPath`: - Allows integration with other packages. - Facilitates tests. - Using `ascii_art_tree` to show the output tree, with styles `dots` (original) and `elegant`. - CLI: - Added options: - `--regexp`: to use `RegExp` to match the target import. - `--all`: to find all the import paths. - `-q`: for a quiet output (only displays found paths). - `-s`: strips the search root directory from displayed import paths. - `--elegant`: use `elegant` style for the output tree. - `--dots`: use `dots` style for the output tree. - Improved help with examples. - Updated `README.md` to show CLI and Library usage. - Added tests and coverage (80%). - Added GitHub Dart CI. - Updated dependencies compatible with Dart `2.14.0` (was already dependent to SDK `2.14.0`): - sdk: '>=2.14.0 <3.0.0' - package_config: ^2.1.0 - path: ^1.8.3 - ascii_art_tree: ^1.0.2 - test: ^1.21.4

.github/workflows/dart.yml

jakemac53 · 2023-09-05T14:10:09Z

.github/workflows/dart.yml

+      - name: Install dependencies
+        run: dart pub get
+      - name: Upgrade dependencies
+        run: dart pub upgrade


This is not necessary - a get always fetches the latest versions unless there is a pubspec.lock present.

Either way we should only do one of a get or upgrade though.

In theory I agree with you, until I had some real world issues.

I only have included the upgrade command because in some cases the GitHub workflow has come cached packages and is not getting the last version for some pre-installed packages.

dart pub get only gets the last version if it's downloading the package for the first time.

dart pub get

If the system cache doesn’t already contain the dependencies, dart pub get updates the cache, downloading dependencies if necessary.

From:
https://dart.dev/tools/pub/cmd/pub-get

When running dart pub get, if a dependency is already in the cache and matches the constraint, it won't download a newer version.

That isn't my experience with how it works, but I am fine with using dart pub upgrade. We definitely don't need both though (just remove the get then).

README.md

jakemac53 · 2023-09-05T14:19:32Z

analysis_options.yaml

+# (the recommended set includes the core lints).
+# The core lints are also what is used by pub.dev for scoring packages.
+
+include: package:lints/recommended.yaml


Lets use this one:

Suggested change

include: package:lints/recommended.yaml

include: package:dart_flutter_team_lints/analysis_options.yaml

(will also need to add a dev dependency)

bin/import_path.dart

lib/src/import_path_base.dart

jakemac53 · 2023-09-05T15:24:52Z

lib/src/import_path_base.dart

+    for (var import in newImports) {
+      if (_matchesImport(importToFind, import)) {
+        var foundPath = [...parents, import.toString()];
+        found.add(foundPath);


Consider creating a ChildNode type, which has a pointer to its parent node (but not the other way). This should avoid the need for lists entirely for the paths, which will avoid extra copying around like this. You could instead return iterables, since it's easy to crawl from a child up to all its parents.

We could actually represent a more complex graph that way too, if we allowed multiple parents. And possibly have a mode where we would serve a visual graph or output a graph in dot format? (not for now, but could be a nice follow up).

A representation like that would also resolve the issue I described above, finding an already "walked" node would actually mean just adding the current node to its parents.

Fixed with the use of package graph_explorer

Fwiw, I tested this using my fake package generator tool, which basically just generates packages with really complex import graphs for worst case scenarios. It seems to perform just fine in the shortest path mode 👍.

When there are many paths the --all mode definitely struggles a lot (took several minutes in my case, I gave up after 5). I think there could definitely be a lot of optimization there - ultimately the paths are still being represented with separate lists from what I could tell, which involves a huge amount of duplicate work. They should be represented as Iterables instead, which can be lazily printed and constructed.

I think the model for output in that mode should also really be a graph format (something like the DOT language for graphviz), and we could even then do something similar to the pubviz package does, creating/serving an HTML page for viewing in a browser. The console is just really not a good mechanism for efficiently visualizing large graphs 🤣. In general this format would also be more useful for tools than JSON (for the same reasons, orders of magnitude smaller representation).

I could also envision possibly some sort of interactive console mode, where you can say "show me the next path". If we can lazily materialize the paths, then we can always only show the first X (maybe 10?) and then have a sort of paginated setup where you can ask for more. That way it won't just hang forever, or spit out an infinite stream of crud to the screen :).

This should all be a follow up if anything, for many cases the existing --all implementation will probably work well enough. But I do think it should possibly print a warning that on large programs it might not complete in reasonable time.

Can I test your generator? So I can try to find the bottleneck in the code, beside that parents list?

https://gist.github.com/jakemac53/890f7dac88419f176e964458f9a12f7e

But, this is a fundamental flaw that you won't be able to optimize yourself out of I am quite confident :).

lib/src/import_path_base.dart

…rser`: - `ImportParser`: - Added faster parser. - Added support to conditional imports. - `ImportPathScanner`: - Added support to final all the target imports. - Optimized resolution of paths graph. - CLI: - `--format`: Defines the style for the output tree (elegant, dots, json). - `--fast`: to enable a fast import parser. - ascii_art_tree: ^1.0.5 - graph_explorer: ^1.0.0

gmpassos · 2023-09-11T09:55:47Z

I have made the following changes:

Moved code to class ImportPath, ImportPathScanner and ImportParser:
ImportParser:
- Added faster parser.
- Added support to conditional imports.
ImportPathScanner:
- Added support to final all the target imports.
- Optimized resolution of paths graph.
CLI:
- --format: Defines the style for the output tree (elegant, dots, json).
- --fast: to enable a fast import parser.
ascii_art_tree: ^1.0.5
graph_explorer: ^1.0.0
- Using this package to perform graph operations and path scan.

.github/workflows/dart.yml

jakemac53 · 2023-09-12T17:13:38Z

AUTHORS


-Jacob MacDonald <[email protected]>
+Graciliano M. Passos: gmpassos @ GitHub


Please add me back :)

Sorry, I was editing other package and edited the wrong file.
Do you want to remove your email? And add just @ GitHub?

CHANGELOG.md

jakemac53 · 2023-09-12T17:23:06Z

README.md

+[![Code size](https://img.shields.io/github/languages/code-size/jakemac53/import_path?logo=github&logoColor=white)](https://github.com/jakemac53/import_path)
+[![License](https://img.shields.io/github/license/jakemac53/import_path?logo=open-source-initiative&logoColor=green)](https://github.com/jakemac53/import_path/blob/master/LICENSE)
+
+A tool to find the shortest import path or listing


Suggested change

A tool to find the shortest import path or listing

A tool to find the shortest import path or list

jakemac53 · 2023-09-12T17:50:55Z

lib/src/import_path_base.dart

+/// Base class [ImportPath], [ImportPathScanner] and [ImportParser].
+abstract class ImportWidget {
+  /// If `true`, we won't call [printMessage].
+  final bool quiet;


We shouldn't have to plumb this through everywhere - is the MessagePrinter abstraction enough? Can we just have a message printer that does nothing?

We could also consider using package:logging instead so we would have log levels etc, which might help manage this stuff more elegantly (then we don't need a messagePrinter field here either, we could just create our own Logger or use a global one).

messagePrinter is not a logging mechanism, it's an output mechanism.

jakemac53 · 2023-09-12T18:06:41Z

lib/src/import_path_base.dart

+
+  /// Resolves the [searchRoot] using [from] parent.
+  /// See [commonRootDirectories].
+  String resolveSearchRoot() {


Can you explain more what the point of this actually is? In particular with regards to commonRootDirectories?

This seems like new logic but I am having a very hard time following the intentions of the code.

If the goal is to find the root of the package that from lives in, we could use the package config to better do that and show paths relative to that package root without the assumptions we have here?

jakemac53 · 2023-09-12T18:39:28Z

lib/src/import_path_base.dart

+  /// Performs the imports search and returns the tree.
+  /// - [style] defines [ASCIIArtTree] style. Default: [ASCIIArtTreeStyle.elegant]
+  /// - See [searchPaths] and [ASCIIArtTree].
+  Future<ASCIIArtTree?> search(


I don't think this method should be on this class - at most it could be an extension method? I don't think its something that should really be overridden etc, and it's tightly coupling this class to the ASCIIArtTree itself which isn't a good separation of concerns.

I would probably rename searchPaths to just search, and drop this (its only used in one place and trivial to inline there).

jakemac53 · 2023-09-12T18:42:48Z

lib/src/import_path_base.dart

+  /// Executes the import search and prints the results.
+  /// - [style] defines the output format. Default: [ImportPathStyle.elegant]
+  /// - See [search], [printMessage] and [ASCIIArtTree].
+  Future<ASCIIArtTree?> execute(


I also feel like this could be an extension method, or just inlined into the bin/ script.

It is also weird that it both returns the ASCIIArtTree and prints out the paths, but not necessarily in ascii format.

jakemac53 · 2023-09-12T18:43:54Z

lib/src/import_path_base.dart

+  /// Performs the imports search and returns the found import paths.
+  /// - [packageDirectory] is used to resolve . Default: [Directory.current].
+  /// - Uses [ImportPathScanner].
+  Future<List<List<String>>> searchPaths({Directory? packageDirectory}) async {


Mentioned above but I do really think that List<List> is the wrong type here. Given this is a public API I am a bit hesitant to expose that.

You could type it as Future<Iterable<Iterable<String>>> which would give more flexibility to change it to lazy iterables later on?

I vote for Future<Iterable<List<String>>>, since List<String> is a path, won't be possible to be lazy.

The path can be lazy though - that is what I am trying to say :). But it cannot be if it is represented by a List.

It would certainly take some work to get there, but fundamentally each path can be created just by doing a traversal of nodes. I am not sure I can fully explain what I have in mind over github comments though lol 🤣 .

We could also just consider actually exposing the graph representation more directly? If you were given the root of the tree, with only the edges that actually lead to the URI that you are searching for present, that would be a compact representation that you could pretty easily convert into anything else.

jakemac53 · 2023-09-12T18:47:35Z

lib/src/import_path_parser.dart

+
+/// Dart import parser.
+/// Parses for import directives in Dart files.
+class ImportParser extends ImportWidget {


I can't see any difference using the faster parser - and it's a lot of extra stuff to maintain. Ultimately, its only going to be a constant performance factor improvement and we are dealing with something exponential in its complexity, so a constant factor doesn't help. This implementation still looks at the whole file anyways though.

So I think we should drop the fast parser mode (or if you want, you could make it injectable or something and have your own implementation?). I don't want to support this parser ultimately, unless it made a really large difference in performance.

Testing with 2 real projects, the difference with the faster parser is significant. It's 3x faster.

That makes sense that its more noticeable in a real project, I realized that my pet project libraries only have directives 🤣 .

But, while it might be faster at parsing, that isn't overall the slow thing here, and I really don't want to own/maintain a regex based parser.

If we just allow this class to accept an ImportParser (which should really only do String -> dependencies conversion), then you could own this regex based one and use it in your own scenarios?

Well, it's a very simple RegExp. The code finds the last import, then there's a fallback to the full parser in case of error. The idea is not to give a perfect fast parser, just an alternative.

We are using only the fast parser here for out projects. (already using the tool here).

FYI:

dart-lang/sdk#53498

Well, it's a very simple RegExp. The code finds the last import, then there's a fallback to the full parser in case of error. The idea is not to give a perfect fast parser, just an alternative.

We are using only the fast parser here for out projects. (already using the tool here).

But it will still have to be updated as the language evolves (we have multiple proposals out currently that could affect this, they may not land, but I don't have a lot of time to spend maintaining this).

FYI:

dart-lang/sdk#53498

I definitely think it would be great if we could get some better support from the SDK here, we do this in a lot of places.

Co-authored-by: Jacob MacDonald <[email protected]>

…0.6 ; graph_explorer: ^1.0.1

gmpassos added 8 commits September 3, 2023 18:12

pubspec.yaml: adjusted dev_dependencies:

eaba85a

Clean code

4fb8a35

README.md: add badges

1b686fa

README.md: add badges

91f38c8

Dart CI: added windows and macos tests

a7f54f6

README.md: update package description

d341af6

ascii_art_tree: ^1.0.4

5e80367

jakemac53 reviewed Sep 5, 2023

View reviewed changes

gmpassos added 13 commits September 5, 2023 21:46

Using package args to resolve command flags and usage.

01b94b0

README.md: adjust license link

43e8940

ImportPath: make field importToFind final and Object.

9fd4252

only show ImportPath

891a1b5

New class ImportToFind for import matching.

359f31f

ImportPath: resolve field searchRoot during construction.

cc2a7f1

ImportPath: make _stripSearchRoot private.

bed45b8

Added enum ImportPathStyle (dots, elegant, json) ; added JSON output.

92636b2

ImportPath: fix style parameter documentation.

1793d72

ImportPath: fix style parameter documentation.

faeb219

_searchImportPaths: rename node to currentNode

ae1ddb0

rollback license.

944b3e7

jakemac53 reviewed Sep 12, 2023

View reviewed changes

gmpassos and others added 2 commits September 12, 2023 17:04

Update CHANGELOG.md

750acb1

Co-authored-by: Jacob MacDonald <[email protected]>

ImportParser.importsFor: added internal cache ; ascii_art_tree: ^1.…

6e27209

…0.6 ; graph_explorer: ^1.0.1

	include: package:lints/recommended.yaml
	include: package:dart_flutter_team_lints/analysis_options.yaml


		Jacob MacDonald <[email protected]>
		Graciliano M. Passos: gmpassos @ GitHub

	A tool to find the shortest import path or listing
	A tool to find the shortest import path or list

v1.2.0 - findAll, ImportPath class and use of ascii_art_tree. #3

Are you sure you want to change the base?

v1.2.0 - findAll, ImportPath class and use of ascii_art_tree. #3

Uh oh!

Conversation

gmpassos commented Sep 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmpassos Sep 5, 2023 • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmpassos Sep 11, 2023 • 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!

Uh oh!

gmpassos commented Sep 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

gmpassos commented Sep 3, 2023 •

edited

Loading

gmpassos Sep 5, 2023 •

edited

Loading

gmpassos Sep 11, 2023 •

edited

Loading

gmpassos commented Sep 11, 2023 •

edited

Loading