Code review for existing code #4

jathak · 2019-02-04T21:08:12Z

No description provided.

Also adds some tests and better comments.

The default is now to only migrate files explicitly passed as entrypoints. Passing the `-r` flag will migrate all dependencies recursively. We still need to look at all dependencies even if we're not migrating them. To make this cleaner, the migration now has two passes. First, we build the dependency graph by parsing the StylesheetApi (consisting of the variables, functions, mixins, and imports) of every sheet in the graph. We then run the actual migration of the entrypoints (and, if desired, their dependencies).

Renames the package to better match the GitHub repo name. Also removes unnecessary dependencies and upgrades the Sass version.

Errors while migrating now throw instead of just returning false through the visitor.

Each set of source files to migrate is now represented as an HRX archive (see test/migrations/README.md for more information).

The path resolver now follows the actual algorithm that Sass uses, so it should properly handle partials and relative paths now.

When built-in functions are used within a source file, the migrator will now add an import for the corresponding built-in module and add a namespace to the function call. As part of this change, the migrator now finishes the file it is on before starting the migration of any dependencies. This eliminates the need to store in-progress migration data for more than one file at a time.

Checks the analyzer, formatting, and tests

When an imported variable is shadowed by a local variable, we should avoid namespacing it.

nex3

This is looking really good! I have a bunch of comments, but most of them are small stylistic stuff. The only big thing is the possibility of switching from StylesheetApi to something more like global scope tracking.

.travis.yml

README.md

bin/sass_module_migrator.dart

lib/src/base_visitor.dart

nex3 · 2019-02-07T01:33:03Z

test/migrations/variables.hrx

+}
+
+<==> expected/four.scss
+$c: 4px;


At some point, it would be good to remove variable declarations that are used to configure an import. Maybe file an issue for that?

test/migrator_test.dart

test/patch_test.dart

nex3 · 2019-02-07T01:38:25Z

test/patch_test.dart

+import 'package:test/test.dart';
+
+void main() {
+  test("single patch applied", () {


Generally speaking, test names should be full sentences that succinctly describe the expected behavior. For example, "a single patch replaces the original text".

jathak · 2019-02-07T06:01:58Z

Thank you for such a thorough review! I'll probably work through these in stages as I have time.

Highlights: - The migrator now extends RecursiveStatementVisitor, eliminating the need for BaseVisitor - Migration state for each stylesheet is now stored in StylesheetMigration, which keeps track of member scope, namespaces, and patches. - The migrator now always runs on all dependencies, with the command-line options now only used to determine which migrations are written to disk. This simplifies the migrator logic. - Since dependencies are migrated at the time the import rule is migrated, the global members of the dependency are forwarded onto the StylesheetMigration, which allows the namespacing of implicit dependencies to avoid the complicated logic of examining the entire dependency graph each time. - The tests now use test_descriptor to avoid the need for mocks.

jathak · 2019-02-12T06:40:48Z

I refactored away BaseVisitor and StylesheetApi, changing the migrator itself to run a single pass on all dependencies, with each stylesheet's migration state represented by a new StylesheetMigration class. This also allowed the scoping and implicit import tracking to be simplified in my mind.

I believe I've addressed most of your comments, aside from more tightly-scoped tests, which I'll work on in a separate branch. Let me know if anything needs clarification. Thanks!

nex3

Really great improvements!

nex3 · 2019-02-13T00:05:15Z

lib/src/local_scope.dart

+  final LocalScope parent;
+
+  /// Variables defined in this scope.
+  final Set<String> variables = normalizedSet();


You can omit Set<String> here; it'll be inferred from normalizedSet().

lib/src/migrator.dart

nex3 · 2019-02-13T00:42:42Z

lib/src/migrator.dart

+
+class _Migrator extends RecursiveStatementVisitor implements ExpressionVisitor {
+  /// List of all migrations for files touched by this run.
+  p.PathMap<StylesheetMigration> _migrations = p.PathMap();


Suggested change

p.PathMap<StylesheetMigration> _migrations = p.PathMap();

final _migrations = p.PathMap<StylesheetMigration>();

You can avoid redundantly declaring p.PathMap by putting the type annotation on the right, and it's always a good idea to make instance variables final if they aren't ever reassigned. Same below.

nex3 · 2019-02-13T00:43:11Z

lib/src/migrator.dart

+  /// List of all migrations for files touched by this run.
+  p.PathMap<StylesheetMigration> _migrations = p.PathMap();
+
+  /// List of migrations in progress. The last item in the current migration.


Suggested change

/// List of migrations in progress. The last item in the current migration.

/// List of migrations in progress. The last item is the current migration.

nex3 · 2019-02-13T00:43:20Z

lib/src/migrator.dart

+  /// List of migrations in progress. The last item in the current migration.
+  List<StylesheetMigration> _activeMigrations = [];
+
+  StylesheetMigration get _currentMigration =>


Nit: document this.

lib/src/migrator.dart

nex3 · 2019-02-13T07:40:14Z

lib/src/migrator.dart

+    _currentMigration.namespaces[importMigration.path] =
+        namespaceForPath(import.url);
+
+    // Ensure that references to members transient dependencies can be


Suggested change

// Ensure that references to members transient dependencies can be

// Ensure that references to members' transient dependencies can be

nex3 · 2019-02-13T07:41:16Z

lib/src/migrator.dart

+    // namespaced.
+    _currentMigration.variables.addEntries(importMigration.variables.entries);
+    _currentMigration.mixins.addEntries(importMigration.mixins.entries);
+    _currentMigration.functions.addEntries(importMigration.functions.entries);


Are these necessary to track now that you're also tracking the global scope? In fact, won't they be misleading, since once the current stylesheet is migrated to @use it won't make these members transitively visible?

By adding the global members from imports to the current stylesheet, it lets us find existing references to transitively-visible members that need to be namespaced with the transitive dependency, not the direct one. This allows namespaceForNode to return the correct namespace (adding it if it hasn't been @use-d) by looking at the source path of the VariableDeclaration, MixinRule, or FunctionRule in one of these dictionaries.

Oh, because the scope is bound to a StylesheetMigration? Maybe it should be a member of _Migrator instead... after all, the same global scope is shared across all stylesheets.

I don't think a single global scope would work when the migrator is running on multiple entrypoints. The scope would need to be cleared between each entrypoint, but the stored global members for each dependency need to remain available in case a future entrypoint also re-uses them.

I suspect that you'll end up needing to abandon shared state across entrypoints. The way names are resolved in an individual file is highly dependent on the global state at the time it's loaded, so I suspect you'll run into lots of nasty edge cases if you don't treat each entrypoint as an entirely separate run.

Oh, okay. That makes sense. I'll change it so the scoping information is just stored in _Migrator.

What should I do in the case where the user wants to migrate two entrypoints that share a dependency, but the dependency would be migrated differently depending on which entrypoint is loading it?

Probably just throw an error.

nex3 · 2019-02-13T07:43:14Z

lib/src/migrator.dart

+    _currentMigration.localScope = _currentMigration.localScope.parent;
+  }
+
+  /// Adds a namespace to any function calls that require them.


Suggested change

/// Adds a namespace to any function calls that require them.

/// Adds a namespace to any function call that requires it.

(Also below)

nex3 · 2019-02-13T07:46:48Z

lib/src/stylesheet_migration.dart

+  /// Finds the namespace for the stylesheet containing [node], adding a new use
+  /// rule if necessary.
+  String namespaceForNode(SassNode node) {
+    var nodePath = node.span.sourceUrl.path;


Suggested change

var nodePath = node.span.sourceUrl.path;

var nodePath = p.fromUri(node.span.sourceUrl);

A file: URL's path component isn't always identical to a filesystem path.

jathak · 2019-02-13T22:13:18Z

Addressed remaining comments. See my explanation of why we need to copy the global members of an import into the current stylesheet's member maps here

nex3

Don't forget my comments on the tests!

nex3 · 2019-02-14T00:51:39Z

lib/src/migrator.dart

+    // namespaced.
+    _currentMigration.variables.addEntries(importMigration.variables.entries);
+    _currentMigration.mixins.addEntries(importMigration.mixins.entries);
+    _currentMigration.functions.addEntries(importMigration.functions.entries);


Oh, because the scope is bound to a StylesheetMigration? Maybe it should be a member of _Migrator instead... after all, the same global scope is shared across all stylesheets.

nex3 · 2019-02-14T00:52:52Z

lib/src/migrator.dart

+
+class _Migrator extends RecursiveStatementVisitor implements ExpressionVisitor {
+  /// List of all migrations for files touched by this run.
+  final _migrations = p.PathMap();


Suggested change

final _migrations = p.PathMap();

final _migrations = p.PathMap<StylesheetMigration>();

nex3 · 2019-02-14T00:53:08Z

lib/src/migrator.dart

+  final _migrations = p.PathMap();
+
+  /// List of migrations in progress. The last item is the current migration.
+  List<StylesheetMigration> _activeMigrations = [];


Suggested change

List<StylesheetMigration> _activeMigrations = [];

final _activeMigrations = <StylesheetMigration>[];

nex3 · 2019-02-14T00:54:08Z

lib/src/migrator.dart

+        : p.join(p.dirname(_currentMigration.path), path));
+    return _migrations.putIfAbsent(path, () {
+      var migration = StylesheetMigration(path);
+      _migrations[path] = migration;


This is no longer necessary; putIfAbsent takes care of adding the migration once the block is complete.

Previously, when adding a new use rule for an indirect reference to a member of a partial, the underscore would be removed from the rule's URL, but its namespace was calculated based on the full path.

jathak · 2019-03-02T00:45:18Z

I changed the tests to be more tightly scoped and addressed your remaining comments. Let me know if there's anything else I should do before merging #8.

Changes based on review for #4

jathak and others added 18 commits November 9, 2018 17:52

Initial scaffolding for migrator

d2b0439

Initial support for variable namespacing

69c499e

Adds use rules for transient dependencies

1e16102

Also adds some tests and better comments.

Add support for migrating to configured use rule

5eba179

Add runnable script and misc cleanup

c364bee

Updating formatting of license header

54e2a16

Rename package, cleanup pubspec

09c2eb5

Renames the package to better match the GitHub repo name. Also removes unnecessary dependencies and upgrades the Sass version.

Cleans up BaseVisitor

3c2fa02

Errors while migrating now throw instead of just returning false through the visitor.

Remove AUTHORS file

a37375d

Refactors migration tests.

7e8e053

Each set of source files to migrate is now represented as an HRX archive (see test/migrations/README.md for more information).

Properly resolve paths for imports

94d6902

The path resolver now follows the actual algorithm that Sass uses, so it should properly handle partials and relative paths now.

Migrate user-defined function references

e1281c5

Adds Travis (#1)

98703e1

Checks the analyzer, formatting, and tests

Prevents namespacing shadowed imported variables (#2)

60c35c1

When an imported variable is shadowed by a local variable, we should avoid namespacing it.

Migrate mixin include rules (#3)

f49618c

Merge branch 'master' into review

72c81a0

jathak requested a review from nex3 February 4, 2019 21:08

nex3 requested changes Feb 7, 2019

View reviewed changes

jathak added 5 commits February 12, 2019 05:32

Misc cleanup and docs

1c4efe4

Merge branch 'refactor' into review

5626a65

Remove unnecessary type annotation

18f5a82

Merge branch 'refactor' into review

84b2724

jathak mentioned this pull request Feb 12, 2019

Remove variable declarations that are used to configure an import #9

Closed

jathak added 2 commits February 12, 2019 06:45

Fix off-by-one error in tests

39dcf27

Merge branch 'refactor' into review

f82a308

jathak added 2 commits February 12, 2019 07:20

Fix travis

b2f70c0

Merge branch 'refactor' into review

1cf9b21

nex3 requested changes Feb 13, 2019

View reviewed changes

jathak added 2 commits February 13, 2019 22:10

Address new comments on #4.

ffcc5a8

Merge branch 'refactor' into review

1ca78ee

nex3 requested changes Feb 14, 2019

View reviewed changes

jathak added 13 commits February 14, 2019 02:11

Additional fixes

93e9d05

Merge branch 'refactor' into review

aa84384

Move scoping to _Migrator

917bb0f

Merge branch 'refactor' into review

36d689d

Initial work on simplifying tests

e89215b

Actually add the new test files

39be76b

Add a few more simplified tests

cec7ff8

Add todo for supporting files with existing use rules

c92b990

Add test for namespacing built-in functions

52437ab

Fix bug when namespacing indirect references

1f44140

Previously, when adding a new use rule for an indirect reference to a member of a partial, the underscore would be removed from the rule's URL, but its namespace was calculated based on the full path.

Merge branch 'refactor' into review

0212199

Update test README; log filename when test fails

860e989

Merge branch 'refactor' into review

a8243ab

jathak added a commit that referenced this pull request Mar 8, 2019

Merge pull request #8 from sass/refactor

0d28291

Changes based on review for #4

jathak closed this Mar 8, 2019

jathak deleted the review branch March 8, 2019 04:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code review for existing code #4

Code review for existing code #4

jathak commented Feb 4, 2019

nex3 left a comment

nex3 Feb 7, 2019

nex3 Feb 7, 2019

jathak commented Feb 7, 2019

jathak commented Feb 12, 2019

nex3 left a comment

nex3 Feb 13, 2019

nex3 Feb 13, 2019

nex3 Feb 13, 2019

nex3 Feb 13, 2019

nex3 Feb 13, 2019

nex3 Feb 13, 2019

jathak Feb 13, 2019

nex3 Feb 14, 2019

jathak Feb 14, 2019

nex3 Feb 14, 2019

jathak Feb 14, 2019

nex3 Feb 15, 2019

nex3 Feb 13, 2019

nex3 Feb 13, 2019

jathak commented Feb 13, 2019

nex3 left a comment

nex3 Feb 14, 2019

nex3 Feb 14, 2019

nex3 Feb 14, 2019

nex3 Feb 14, 2019

jathak commented Mar 2, 2019

	p.PathMap<StylesheetMigration> _migrations = p.PathMap();
	final _migrations = p.PathMap<StylesheetMigration>();

	/// List of migrations in progress. The last item in the current migration.
	/// List of migrations in progress. The last item is the current migration.

	// Ensure that references to members transient dependencies can be
	// Ensure that references to members' transient dependencies can be

	/// Adds a namespace to any function calls that require them.
	/// Adds a namespace to any function call that requires it.

	var nodePath = node.span.sourceUrl.path;
	var nodePath = p.fromUri(node.span.sourceUrl);

	final _migrations = p.PathMap();
	final _migrations = p.PathMap<StylesheetMigration>();

	List<StylesheetMigration> _activeMigrations = [];
	final _activeMigrations = <StylesheetMigration>[];

Code review for existing code #4

Code review for existing code #4

Conversation

jathak commented Feb 4, 2019

nex3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jathak commented Feb 7, 2019

jathak commented Feb 12, 2019

nex3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jathak commented Feb 13, 2019

nex3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jathak commented Mar 2, 2019