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

Generalizes to support multiple migrators #16

Merged
merged 7 commits into from Apr 18, 2019

Conversation

Projects
None yet
2 participants
@jathak
Copy link
Member

commented Apr 1, 2019

Resolves #19.

There's now a Migrator class that additional migrators can extend.

Most migrators will want to extend MigratorBase, which handles the tree traversal. For now, MigratorBase only operates on a single stylesheet, with the dependency migration implemented only by ModuleMigrator.

Generalizes to support multiple migrators
There's now a `Migrator` class that additional migrators can extend.

Most migrators will want to extend `MigratorBase`, which handles the
tree traversal. For now, `MigratorBase` only operates on a single
stylesheet, with the dependency migration implemented only by
`ModuleMigrator`.

@jathak jathak requested a review from nex3 Apr 1, 2019

@nex3
Copy link
Contributor

left a comment

Are you planning to have Migrator stay non-recursive? If so, what's the plan for migrating entire projects at once?

Show resolved Hide resolved bin/sass_migrator.dart Outdated
Show resolved Hide resolved bin/sass_migrator.dart Outdated
Show resolved Hide resolved bin/sass_migrator.dart Outdated
Show resolved Hide resolved bin/sass_migrator.dart Outdated
Show resolved Hide resolved bin/sass_migrator.dart Outdated
Show resolved Hide resolved lib/src/migrator_base.dart Outdated
Show resolved Hide resolved lib/src/migrators/module.dart Outdated
Show resolved Hide resolved lib/src/migrators/module.dart Outdated
Show resolved Hide resolved lib/src/migrators/module.dart Outdated
Show resolved Hide resolved test/migrator_test.dart Outdated

jathak added some commits Apr 4, 2019

Generalizes more of the migrator
--migrate-deps is now a global flag, so additional migrators should not
need to re-implement dependency migration unless they need additional
special logic like the module migrator needs.

The generic migration classes are now split into Migrator and
MigrationVisitor. Each Migrator is a Command and should contain state
for the entire migration run. Most migrators will also want to create a
private subclass of MigrationVisitor that they call from
Migrator.migrateFile. A new instance of the MigrationVisitor will be
created for each stylesheet being migrated, so it contains
per-stylesheet state, similar to what StylesheetMigration used to be for
the module migrator.
@jathak

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

I've updated this to now have the generic migration support split into Migrator, which extends Command and contains state for the entire migration run, and MigrationVisitor, which migrators subclass and use to traverse the AST of a single stylesheet.

I've also made the --migrate-deps flag global. The module migrator has some special logic here since it needs to visit all dependencies even if they're not being migrated, but any migrators that don't rely on cross-stylesheet information should be able to support migrating dependencies without any additional code.

@jathak jathak requested a review from nex3 Apr 5, 2019

@nex3
Copy link
Contributor

left a comment

This is shaping up really nicely! Thanks for bearing with my intense reviews 😅.

Show resolved Hide resolved lib/runner.dart
Show resolved Hide resolved lib/runner.dart Outdated
Show resolved Hide resolved lib/runner.dart Outdated
Show resolved Hide resolved lib/src/migration_visitor.dart Outdated
Show resolved Hide resolved lib/src/migration_visitor.dart Outdated
Show resolved Hide resolved lib/src/migrators/module.dart Outdated
Show resolved Hide resolved lib/src/utils.dart
Show resolved Hide resolved lib/src/utils.dart Outdated
Show resolved Hide resolved lib/src/utils.dart Outdated
Show resolved Hide resolved pubspec.yaml Outdated
Iterate in response to review.
The migrator now uses URLs instead of paths, and loads files using the
FilesystemImporter.

`Migrator.migrated` has been removed in favor of having
`Migrator.migrateFile` and `MigrationVisitor.run` return it.
Show resolved Hide resolved lib/runner.dart Outdated
Show resolved Hide resolved lib/runner.dart Outdated
Show resolved Hide resolved lib/src/migration_visitor.dart Outdated
visitStylesheet(stylesheet);
var results = getMigratedContents();
if (results != null) {
migrator.migrated[path] = results;

This comment has been minimized.

Copy link
@nex3

nex3 Apr 9, 2019

Contributor

That's a good question. I'd say make the migrated field private to MigrationVisitor and give it total responsibility for handling the output, but you need to pass it through newInstance() somehow...

What if we restructure this so that you don't need to construct multiple visitors at all? Rather than passing a URL to the constructor, pass it to run(). That also means that when we get around to de-duplicating repeated imports/uses (which will probably be necessary for large-scale performance), there's a natural place for that as well.

This does mean that per-file state can't just be stored as final fields on the subclass. But you can override visitStylesheet() to set it temporarily:

@override
void visitStylesheet(StylesheetNode node) {
  var oldNamespaces = _namespaces;
  var oldAdditionalUseRules = _additionalUseRules;
  var oldConfigurableVariables = _configurableVariables;
  _namespaces = {};
  _additionalUseRules = Set();
  _configurableVariables = normalizedSet();
  super.visitStylesheet(node);
  _namespaces = oldNamespaces;
  _additionalUseRules = oldAdditionalUseRules;
  _configurableVariables = oldConfigurableVariables;
}

This is basically how Dart Sass's EvaluateVisitor works. WDYT?

Show resolved Hide resolved lib/src/migration_visitor.dart Outdated
Show resolved Hide resolved lib/src/migration_visitor.dart Outdated
Show resolved Hide resolved lib/src/utils.dart
Show resolved Hide resolved test/migrator_utils.dart Outdated
@jathak

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

Changed MigrationVisitor to reuse a single instance for dependencies.

Previously, the module migrator needed to access the _configurableVariables of its dependencies' visitors. In the process of changing this, I found #28 and (mostly) fixed this in my changes (though it doesn't yet remove an unnecessary use).

@jathak jathak requested a review from nex3 Apr 12, 2019

@nex3
Copy link
Contributor

left a comment

Would it be possible to factor out the configurable-variables change into a separate PR? It's kind of tough to review this when it's changing multiple things at once.

Show resolved Hide resolved lib/src/migration_visitor.dart Outdated
Show resolved Hide resolved lib/src/migration_visitor.dart Outdated
Reuse single MigrationVisitor
A single MigrationVisitor is now reused instead of constructing a new
instance for each dependency.

This also removes support for configurable variables, since the previous
implementation of this was both hard to implement with a single
MigrationVisitor and incorrect when the configurable variables were
declared in an indirect dependency.

A subsequent PR will add this back with a new implementation that fixes
the bug with indirect dependencies.

@jathak jathak force-pushed the generalization branch from 204bc24 to 8bd5417 Apr 17, 2019

@jathak

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

The old behavior would be hard to implement with a single MigrationVisitor, so I removed support for configurable variables entirely and will add them back with the new implementation in a separate PR.

@jathak jathak requested a review from nex3 Apr 17, 2019

@nex3

nex3 approved these changes Apr 18, 2019

final _globalFunctions = normalizedMap<FunctionRule>();

/// Namespaces of modules used in this stylesheet.
Map<Uri, String> _namespaces = {};

This comment has been minimized.

Copy link
@nex3

nex3 Apr 18, 2019

Contributor

I think you can leave off the initializer here; this value will never be used, since it's always going to be set in visitStylesheet().

if (results == null) return null;
var semicolon = _currentUrl.path.endsWith('.sass') ? "" : ";";
var uses = _additionalUseRules.map((use) => '@use "$use"$semicolon\n');
return uses.join("") + results;

This comment has been minimized.

Copy link
@nex3

nex3 Apr 18, 2019

Contributor
Suggested change
return uses.join("") + results;
return uses.join() + results;
_localScope = _localScope.parent;
}

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

This comment has been minimized.

Copy link
@nex3

nex3 Apr 18, 2019

Contributor
Suggested change
/// Adds a namespace to any function call that require it.
/// Adds a namespace to any function call that requires it.
Show resolved Hide resolved lib/src/utils.dart

@jathak jathak merged commit 29e9ffb into master Apr 18, 2019

@jathak jathak deleted the generalization branch Apr 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.