Fix issue #560 #563

Merged
merged 2 commits into from Oct 6, 2012

Conversation

Projects
None yet
3 participants
@sleeper
Contributor

sleeper commented Sep 30, 2012

Attempt to fix issue #560.

Note that for the time being only blank lines are considered, but thinking of it we should add perhaps more cases.
Although, if I've time, I'd like at how we cab have more granular tests to be able to easily test out these kind of changes.

@sleeper

This comment has been minimized.

Show comment Hide comment
@sleeper

sleeper Oct 2, 2012

Contributor

The added bonus fix #565 by treating the case of filename collision (i.e. 2 files with the same name in 2 different subdirectories).

@sindresorhus, @addyosmani could you have just a slight review to give me your green light before merging ?

Contributor

sleeper commented Oct 2, 2012

The added bonus fix #565 by treating the case of filename collision (i.e. 2 files with the same name in 2 different subdirectories).

@sindresorhus, @addyosmani could you have just a slight review to give me your green light before merging ?

@mklabs

This comment has been minimized.

Show comment Hide comment
@mklabs

mklabs Oct 2, 2012

Contributor

This is great @sleeper Sounds good to me. The only missing thing would be tests to cover that specific filename collision issue.

Contributor

mklabs commented Oct 2, 2012

This is great @sleeper Sounds good to me. The only missing thing would be tests to cover that specific filename collision issue.

cli/tasks/usemin.js
+ // of cached files (we need to treat the filename collision -- i.e. 2 files with same names
+ // in different subdirectories)
+ var filepaths = grunt.file.expand(path.join('**/*') + basename);
+ var filepath = filepaths.filter(function(f) { return(dirname == path.dirname(f));});

This comment has been minimized.

Show comment Hide comment
@mklabs

mklabs Oct 2, 2012

Contributor

Why wrapping return(dirname == path.dirname(f)); into parenthesis?

Also, might seem silly because you don't compare against falsy value but could you do a strict comparison instead === ?

@mklabs

mklabs Oct 2, 2012

Contributor

Why wrapping return(dirname == path.dirname(f)); into parenthesis?

Also, might seem silly because you don't compare against falsy value but could you do a strict comparison instead === ?

This comment has been minimized.

Show comment Hide comment
@sleeper

sleeper Oct 2, 2012

Contributor

OK, will do it as soon as possible ... My learning of Javascript ;)

@sleeper

sleeper Oct 2, 2012

Contributor

OK, will do it as soon as possible ... My learning of Javascript ;)

@sleeper

This comment has been minimized.

Show comment Hide comment
@sleeper

sleeper Oct 2, 2012

Contributor

@mklabs Agreed on the testing part. I've in my backlog to have a deeper look at this part, mainly to be able to test these kind of changes without testing against a full yeoman build output (i.e. trying to be more on the unit test side, and less on the integration side ;))

Contributor

sleeper commented Oct 2, 2012

@mklabs Agreed on the testing part. I've in my backlog to have a deeper look at this part, mainly to be able to test these kind of changes without testing against a full yeoman build output (i.e. trying to be more on the unit test side, and less on the integration side ;))

@mklabs

This comment has been minimized.

Show comment Hide comment
@mklabs

mklabs Oct 2, 2012

Contributor

I'm really happy you did these changes. This tiny little change is fixing that nasty filename collision issue we carry on from the original node-build-script task for usemin.

Regarding the tests and trying to test the usemin task specifically, that's something I tried to do in the usemin test for the node-build-script project: https://github.com/h5bp/node-build-script/blob/master/test/tasks/test-usemin.js

Maybe it can give you some insights. Let me know if I can be of any help, would be more than happy to do so.

Contributor

mklabs commented Oct 2, 2012

I'm really happy you did these changes. This tiny little change is fixing that nasty filename collision issue we carry on from the original node-build-script task for usemin.

Regarding the tests and trying to test the usemin task specifically, that's something I tried to do in the usemin test for the node-build-script project: https://github.com/h5bp/node-build-script/blob/master/test/tasks/test-usemin.js

Maybe it can give you some insights. Let me know if I can be of any help, would be more than happy to do so.

@sleeper

This comment has been minimized.

Show comment Hide comment
@sleeper

sleeper Oct 2, 2012

Contributor

OK. I made the necessary changes and rebased against master.
I'll have a look at the test part ;)

Contributor

sleeper commented Oct 2, 2012

OK. I made the necessary changes and rebased against master.
I'll have a look at the test part ;)

@sleeper

This comment has been minimized.

Show comment Hide comment
@sleeper

sleeper Oct 3, 2012

Contributor

@mklabs i started having a look at testing usemin task (and especially the replace helper). I'm a little bit fighting adding fixtures but I guess I'm starting to understand how it fits together...

Contributor

sleeper commented Oct 3, 2012

@mklabs i started having a look at testing usemin task (and especially the replace helper). I'm a little bit fighting adding fixtures but I guess I'm starting to understand how it fits together...

Fix issue #560 and #565
Treat the case of empty lines in blocks (#560)
as well as the filename collision (#565)
@sleeper

This comment has been minimized.

Show comment Hide comment
@sleeper

sleeper Oct 3, 2012

Contributor

@mklabs tests should be ok now ;)

Contributor

sleeper commented Oct 3, 2012

@mklabs tests should be ok now ;)

@sleeper

This comment has been minimized.

Show comment Hide comment
@sleeper

sleeper Oct 4, 2012

Contributor

Dear all, do I have your green light to merge ?

@mklabs @addyosmani @sindresorhus

Contributor

sleeper commented Oct 4, 2012

Dear all, do I have your green light to merge ?

@mklabs @addyosmani @sindresorhus

@sleeper

This comment has been minimized.

Show comment Hide comment
@sleeper

sleeper Oct 5, 2012

Contributor

Just added unit test and fix of Issue #586.

Contributor

sleeper commented Oct 5, 2012

Just added unit test and fix of Issue #586.

@sleeper

This comment has been minimized.

Show comment Hide comment
@sleeper

sleeper Oct 6, 2012

Contributor

OK, guys .. I merge ! ;)

Contributor

sleeper commented Oct 6, 2012

OK, guys .. I merge ! ;)

sleeper added a commit that referenced this pull request Oct 6, 2012

@sleeper sleeper merged commit 77e7907 into yeoman:master Oct 6, 2012

1 check passed

default The Travis build passed
Details
@sindresorhus

This comment has been minimized.

Show comment Hide comment
@sindresorhus

sindresorhus Oct 9, 2012

Owner

@sleeper Yes, looks good. Sorry for the late reply, was on vacation.

Owner

sindresorhus commented Oct 9, 2012

@sleeper Yes, looks good. Sorry for the late reply, was on vacation.

@sleeper

This comment has been minimized.

Show comment Hide comment
@sleeper

sleeper Oct 9, 2012

Contributor

@sindresorhus No problem. Hope you enjoyed your vacation ;)

Contributor

sleeper commented Oct 9, 2012

@sindresorhus No problem. Hope you enjoyed your vacation ;)

szinya pushed a commit to menthainternet/yeoman that referenced this pull request Sep 17, 2014

sleeper added a commit that referenced this pull request Apr 24, 2015

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