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

Code review for existing code #4

Open
wants to merge 33 commits into
base: empty
from

Conversation

Projects
None yet
2 participants
@jathak
Copy link
Contributor

jathak commented Feb 4, 2019

No description provided.

jathak added some commits Nov 10, 2018

Adds use rules for transient dependencies
Also adds some tests and better comments.
Allows for migration of a single file
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).
Rename package, cleanup pubspec
Renames the package to better match the GitHub repo name.

Also removes unnecessary dependencies and upgrades the Sass version.
Cleans up BaseVisitor
Errors while migrating now throw instead of just returning false through
the visitor.
Refactors migration tests.
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
The path resolver now follows the actual algorithm that Sass uses, so it
should properly handle partials and relative paths now.
Support migrating built-in fns to use modules
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.
Adds Travis (#1)
Checks the analyzer, formatting, and tests
Prevents namespacing shadowed imported variables (#2)
When an imported variable is shadowed by a local variable, we should
avoid namespacing it.

@jathak jathak requested a review from nex3 Feb 4, 2019

@nex3
Copy link

nex3 left a comment

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.

Show resolved Hide resolved .travis.yml Outdated
Show resolved Hide resolved README.md Outdated
Show resolved Hide resolved bin/sass_module_migrator.dart Outdated
Show resolved Hide resolved lib/src/base_visitor.dart Outdated
Show resolved Hide resolved lib/src/base_visitor.dart Outdated
}

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

This comment has been minimized.

@nex3

nex3 Feb 7, 2019

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

Show resolved Hide resolved test/migrator_test.dart Outdated
Show resolved Hide resolved test/migrator_test.dart Outdated
Show resolved Hide resolved test/patch_test.dart
import 'package:test/test.dart';

void main() {
test("single patch applied", () {

This comment has been minimized.

@nex3

nex3 Feb 7, 2019

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

@jathak

This comment has been minimized.

Copy link
Contributor Author

jathak commented Feb 7, 2019

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

jathak added some commits Feb 12, 2019

Various refactors based on review on #4.
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

This comment has been minimized.

Copy link
Contributor Author

jathak commented Feb 12, 2019

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
Copy link

nex3 left a comment

Really great improvements!

final LocalScope parent;

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

This comment has been minimized.

@nex3

nex3 Feb 13, 2019

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

if (ns == null) return;
}
var endName = node.arguments.span.start.offset;
var startName = endName - node.name.length;

This comment has been minimized.

@nex3

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

This comment has been minimized.

@nex3

nex3 Feb 13, 2019

Suggested change Beta
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.

/// 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.

This comment has been minimized.

@nex3

nex3 Feb 13, 2019

Suggested change Beta
/// List of migrations in progress. The last item in the current migration.
/// List of migrations in progress. The last item is the current migration.
/// List of migrations in progress. The last item in the current migration.
List<StylesheetMigration> _activeMigrations = [];

StylesheetMigration get _currentMigration =>

This comment has been minimized.

@nex3

nex3 Feb 13, 2019

Nit: document this.

_currentMigration.namespaceForNode(_currentMigration.functions[name]);
}
if (namespace == null) {
if (!builtInFunctionModules.containsKey(name)) return;

This comment has been minimized.

@nex3

nex3 Feb 13, 2019

I wonder if it might be a good idea to emit a warning here. If the migrator can't tell what a function is referring to, chances are the user invoked it wrong somehow and wants to know about it. Not something you need to do right now, but probably a good idea to add TODOs so you can easily find the places to add warnings down the line.

This comment has been minimized.

@jathak

jathak Feb 13, 2019

Author Contributor

_currentMigration.functions only contains functions defined in the global scope (whether in the current file or in one of its imports), so it's possible this could be referring to a local function.

_currentMigration.namespaces[importMigration.path] =
namespaceForPath(import.url);

// Ensure that references to members transient dependencies can be

This comment has been minimized.

@nex3

nex3 Feb 13, 2019

Suggested change Beta
// Ensure that references to members transient dependencies can be
// Ensure that references to members' transient dependencies can be
// namespaced.
_currentMigration.variables.addEntries(importMigration.variables.entries);
_currentMigration.mixins.addEntries(importMigration.mixins.entries);
_currentMigration.functions.addEntries(importMigration.functions.entries);

This comment has been minimized.

@nex3

nex3 Feb 13, 2019

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?

This comment has been minimized.

@jathak

jathak Feb 13, 2019

Author Contributor

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.

This comment has been minimized.

@nex3

nex3 Feb 14, 2019

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.

This comment has been minimized.

@jathak

jathak Feb 14, 2019

Author Contributor

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.

This comment has been minimized.

@nex3

nex3 Feb 14, 2019

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.

This comment has been minimized.

@jathak

jathak Feb 14, 2019

Author Contributor

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?

This comment has been minimized.

@nex3

nex3 Feb 15, 2019

Probably just throw an error.

_currentMigration.localScope = _currentMigration.localScope.parent;
}

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

This comment has been minimized.

@nex3

nex3 Feb 13, 2019

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

(Also below)

/// 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;

This comment has been minimized.

@nex3

nex3 Feb 13, 2019

Suggested change Beta
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

This comment has been minimized.

Copy link
Contributor Author

jathak commented Feb 13, 2019

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
Copy link

nex3 left a comment

Don't forget my comments on the tests!

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

This comment has been minimized.

@nex3

nex3 Feb 14, 2019

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.


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

This comment has been minimized.

@nex3

nex3 Feb 14, 2019

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

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

This comment has been minimized.

@nex3

nex3 Feb 14, 2019

Suggested change Beta
List<StylesheetMigration> _activeMigrations = [];
final _activeMigrations = <StylesheetMigration>[];
: p.join(p.dirname(_currentMigration.path), path));
return _migrations.putIfAbsent(path, () {
var migration = StylesheetMigration(path);
_migrations[path] = migration;

This comment has been minimized.

@nex3

nex3 Feb 14, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment