Updates #93

Merged
merged 5 commits into from Mar 30, 2013

Conversation

Projects
None yet
2 participants
Contributor

XhmikosR commented Mar 30, 2013

Let me know if you want me to make any changes.

@GoalSmashers GoalSmashers and 1 other commented on an outdated diff Mar 30, 2013

lib/clean.js
@@ -538,11 +538,16 @@ var CleanCSS = {
},
_hueToRgb: function(p, q, t) {
- if (t < 0) t += 1;
- if (t > 1) t -= 1;
- if (t < 1/6) return p + (q - p) * 6 * t;
- if (t < 1/2) return q;
- if (t < 2/3) return p + (q - p) * (2/3 - t) * 6;
+ if (t < 0)
@GoalSmashers

GoalSmashers Mar 30, 2013

Contributor

I prefer the old style as it's easier to read.

@XhmikosR

XhmikosR Mar 30, 2013

Contributor

OK.

@GoalSmashers GoalSmashers and 1 other commented on an outdated diff Mar 30, 2013

lib/clean.js
@@ -286,8 +286,8 @@ var CleanCSS = {
// same 2 values into one
replace(shorthandRegex(2, true), function(match, property, size1, size2, suffix) {
- if (size1 === size2)
- return property + ":" + size1 + suffix;
+ if (size1 == size2)
@GoalSmashers

GoalSmashers Mar 30, 2013

Contributor

As you already know we use === if (and only if) there is a good reason to have it. Please revert.

@XhmikosR

XhmikosR Mar 30, 2013

Contributor

Well, just a line bellow as you can see you use two equal signs to do a similar check.

IMO, you should use two unless you compare with 0.

@GoalSmashers

GoalSmashers Mar 30, 2013

Contributor

There is a chance either size1 or size2 would eq 0. That's why we have ===. There's one check above which do not use them but apparently it should.

@XhmikosR

XhmikosR Mar 30, 2013

Contributor

OK I'll add that then.

@XhmikosR

XhmikosR Mar 30, 2013

Contributor

BTW, 0 can be a lot of variables. That is why I suggested my way, it's cleaner and more consistent.

Contributor

GoalSmashers commented Mar 30, 2013

It's fine beside two comments I made.

Contributor

XhmikosR commented Mar 30, 2013

BTW, Travis fails with my changes. Any idea why? On Windows all the tests pass.

EDIT: Don't merge the PR yet, I will revert temporarily to see what's the cause.

@GoalSmashers GoalSmashers commented on the diff Mar 30, 2013

lib/clean.js
@@ -268,7 +268,7 @@ var CleanCSS = {
return property + ':' + size1;
else if (size1 === size3 && size2 === size4)
return property + ':' + size1 + ' ' + size2;
- else if (size2 == size4)
+ else if (size2 === size4)
@GoalSmashers

GoalSmashers Mar 30, 2013

Contributor

Thanks for adding this. The case is in most places it doesn't make sense to check against 0 where the only possible value is a character string.

Ready to merge?

Contributor

XhmikosR commented Mar 30, 2013

Personally I'd just follow JSHint rules for this (eqeq).

From my side the PR is ready, but I didn't expect this to break the tests on linux.

@GoalSmashers GoalSmashers added a commit that referenced this pull request Mar 30, 2013

@GoalSmashers GoalSmashers Merge pull request #93 from XhmikosR/master
Updates
db207d8

@GoalSmashers GoalSmashers merged commit db207d8 into jakubpawlowicz:master Mar 30, 2013

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment