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

Initial implementation of a division migrator #17

Merged
merged 14 commits into from Apr 23, 2019

Conversation

Projects
None yet
2 participants
@jathak
Copy link
Member

commented Apr 1, 2019

Resolves #20.

This adds a new migrator that migrates the / operator to the division function.

By default, this migrator migrates only cases where the / is known to be division. In cases the / could be division, but whether it is division is not statically known, a warning is emitted instead.

The --aggressive flag can be used to migrate cases like 3 / $x + 1, 3 / $x - 1, and fn() / 3 by assuming that + and - always operate on numbers and that function calls in an expression with / always return numbers.

Note: I'm setting the base for this to the generalization branch for better diffs, but this should be merged after #16.

jathak added some commits Apr 1, 2019

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`.
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.
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.
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.
Initial implementation of a division migrator
This adds a new migrator that migrates the `/` operator to the division
function.

By default, this migrator migrates only cases where the `/` is known to
be division. In cases the `/` could be division, but whether it is
division is not statically known, a warning is emitted instead. The
`--aggressive` flag can be used to migrate cases like `3 / $x + 1`, `3 /
$x - 1`, and `fn() / 3` by assuming that `+` and `-` always operate on
numbers and that function calls in an expression with `/` always return
numbers.

@jathak jathak force-pushed the division-migrator branch from 7483385 to 6156b06 Apr 17, 2019

Show resolved Hide resolved lib/src/migrators/division.dart Outdated
Show resolved Hide resolved lib/src/migrators/division.dart Outdated
Show resolved Hide resolved lib/src/migrators/division.dart
Show resolved Hide resolved lib/src/migrators/division.dart Outdated
Show resolved Hide resolved lib/src/migrators/division.dart Outdated
Show resolved Hide resolved test/migrators/division/always_division.hrx Outdated
Show resolved Hide resolved lib/src/migrators/division.dart Outdated
Show resolved Hide resolved test/migrators/division/always_division.hrx Outdated
Show resolved Hide resolved test/migrators/division/always_division.hrx
Show resolved Hide resolved test/migrators/division/never_division.hrx

@jathak jathak changed the base branch from generalization to master Apr 19, 2019

jathak added some commits Apr 19, 2019

Changes in response to review
Replaced --aggressive with --pessimistic, with the previous behavior for
aggressive now the default. Also made other minor changes in response to
review.
@nex3
Copy link
Contributor

left a comment

This is looking really good 👍. It should be ready to go once slash-list() support is in.

"will be migrated.");
..addFlag('pessimistic',
abbr: 'p',
help: r"Only migrate / expressions that are unambiguously division.");

This comment has been minimized.

Copy link
@nex3

nex3 Apr 19, 2019

Contributor
Suggested change
help: r"Only migrate / expressions that are unambiguously division.");
help: "Only migrate / expressions that are unambiguously division.");
Show resolved Hide resolved lib/src/migrators/division.dart
var expression = node.expression;
if (expression is BinaryOperationExpression &&
expression.operator == BinaryOperator.dividedBy) {
withContext(true, _expectsNumericResult, () {

This comment has been minimized.

Copy link
@nex3

nex3 Apr 19, 2019

Contributor

Ideally, these methods that return booleans wouldn't also produce side effects... is there a way you can make that work? If it's too complicated, though, that's fine.

Show resolved Hide resolved lib/src/migrators/division.dart
@jathak

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2019

Refactored the patching logic from visitBinaryOperationExpression and visitParenthesizedExpression into _visitSlashOperation to get rid of the non-pure _shouldMigrate function and added slash-list() support

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

@nex3

nex3 approved these changes Apr 20, 2019

_patchParensIfAny(expression.right);
}
super.visitBinaryOperationExpression(expression);
_visitSlashOperation(expression, surroundingParens: node);

This comment has been minimized.

Copy link
@nex3

nex3 Apr 20, 2019

Contributor

Rather than passing in surroundingParens, which requires _visitSlashOperation() to understand a lot about what context it might be called in, consider having this code delete the parentheses itself and just call _visitSlashOperation(expression).

Show resolved Hide resolved lib/src/migrators/division.dart Outdated
var start = node.text.span.start.offset;
var end = node.text.span.end.offset;
// Remove `#{` and `}`
addPatch(Patch(node.span.file.span(start, start + 2), ""));

This comment has been minimized.

Copy link
@nex3

nex3 Apr 20, 2019

Contributor

Consider a patchDelete() utility function.

: super(migrateDependencies: migrateDependencies);

/// True when division is allowed by the context the current node is in.
var _isDivisionAllowed = false;

This comment has been minimized.

Copy link
@nex3

nex3 Apr 20, 2019

Contributor

I just realized, this should also be set to true in argument lists for functions and mixins, except for the specific case of new-syntax rgb() et al functions. Feel free to fix after landing this PR, though.

Nothing to migrate!
<==> output/entrypoint.scss
:foo(#{slash-list(6, 3)}) {
b: slash-list(6px, 3px);

This comment has been minimized.

Copy link
@nex3

nex3 Apr 20, 2019

Contributor

Actually, looking at these examples, they're really hard on the eyes. After this lands, can we make the migrator smart enough to only migrate to slash-list() in a non-plain-CSS context? Basically, we should generate slash-list() only if either _isDivisionAllowed is true or one of the arguments (possibly deeply-nested) is an interpolation expression.

@jathak jathak merged commit 02be449 into master Apr 23, 2019

@jathak jathak deleted the division-migrator branch Apr 23, 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.