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

Improvements to the division migrator #41

Merged
merged 5 commits into from May 15, 2019

Conversation

Projects
None yet
2 participants
@jathak
Copy link
Member

commented May 14, 2019

Resolves #32 and resolves #33.

Improvements to the division migrator
Resolves #32 and resolves #33.

@jathak jathak requested a review from nex3 May 14, 2019

if (node.name.asPlain == "rgb" ||
node.name.asPlain == "rgba" ||
node.name.asPlain == "hsl" ||
node.name.asPlain == "hsla") {

This comment has been minimized.

Copy link
@nex3

nex3 May 14, 2019

Contributor

It would be great to remove some of this nesting... maybe factor this out into bool _tryColorFunction(FunctionExpression node) that can just return false every time it determines that the function doesn't need special handling?

channels.separator == ListSeparator.space &&
channels.contents.length == 3 &&
channels.contents[2] is BinaryOperationExpression) {
var red = channels.contents[0];

This comment has been minimized.

Copy link
@nex3

nex3 May 14, 2019

Contributor

Using red et al is a little confusing, because this same path is used for hsl() colors. Maybe channel1 et al instead?

addPatch(patchBefore(node, "slash-list("));
addPatch(patchAfter(node, ")"));
_visitSlashListArguments(node);
if (_isDivisionAllowed || _containsInterpolation(node)) {

This comment has been minimized.

Copy link
@nex3

nex3 May 14, 2019

Contributor

Maybe add a comment here about why we don't always want to use slash-list().

// Function calls
o: identity(divide(6px, 3px));
p: rgba(10, 20, divide(30, 2), 0.5);
q: rgb(10 20 divide(30, 2) / 0.5);

This comment has been minimized.

Copy link
@nex3

nex3 May 14, 2019

Contributor

This actually isn't a valid translation; because divide() isn't a literal number, this / is always interpreted as division, which means the metadata about what the right-hand side used to be gets lost before it reaches rgb(). (This is the kind of nasty confusing edge case that makes the current / semantics so awful.)

I think the ideal translation here is probably rgb(10, 20, divide(30, 2), 0.5), but it's also reasonable to do rgb(10 20 slash-list(divide(30, 2), 0.5)) if that's easier.

b: slash-list(6px, 3px);
c: slash-list(6px, 3px, 2);
:foo(#{6/3}) {
b: 6px / 3px;

This comment has been minimized.

Copy link
@nex3

nex3 May 14, 2019

Contributor

Now that these are being preserved as /, it's probably a good idea to add additional test cases where they're assigned to variables so we can verify that they're converted to slash-list() when necessary.

@jathak jathak requested a review from nex3 May 14, 2019

@nex3

nex3 approved these changes May 15, 2019

bool _tryColorFunction(FunctionExpression node) {
if (!["rgb", "rgba", "hsl", "hsla"].contains(node.name.asPlain)) {
return false;
}

This comment has been minimized.

Copy link
@nex3

nex3 May 15, 2019

Contributor

Consider adding empty lines after each return false to visually emphasize each distinct check.

}
var last = channels.contents.last as BinaryOperationExpression;
if (last.left is! NumberExpression || last.right is! NumberExpression) {
_patchSpacesToCommas(channels);

This comment has been minimized.

Copy link
@nex3

nex3 May 15, 2019

Contributor

It's probably a good idea to provide an example of why this is necessary here. A really dedicated reader who's wondering what this is for could remove this code and see what tests fail, but it's better to save them the trouble.

@nex3

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

Great work!

@nex3

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

Oh, one other thing I forgot to mention: now that we're doing alpha releases, anything that changes the user-visible API surface of the package should get at least a mention in the CHANGELOG.

@jathak jathak merged commit dd3bc85 into master May 15, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@jathak jathak deleted the division-migrator-improvements branch May 15, 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.