Skip to content

Add file-utils feature into remote() function#417

Closed
jdespatis wants to merge 3 commits intoyeoman:masterfrom
jdespatis:file-utils-for-remote
Closed

Add file-utils feature into remote() function#417
jdespatis wants to merge 3 commits intoyeoman:masterfrom
jdespatis:file-utils-for-remote

Conversation

@jdespatis
Copy link
Contributor

Push #378 further, in order to also implement file-utils logic into remote() function

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) when pulling 51e69e6 on jdespatis:file-utils-for-remote into 248e469 on yeoman:master.

@SBoudrias
Copy link
Member

Hi there, thanks for the PR.

Could you add unit test?

@jdespatis
Copy link
Contributor Author

@SBoudrias I was fearing the question..
The fact is I have no time now for some period, so maybe you can accept this PR and add unit tests afterwards ?
or you can cherry pick this commit and add unit tests to perform a new PR, no problem about that

@ghost ghost assigned passy Nov 27, 2013
@addyosmani
Copy link
Member

@passy is going to try authoring the tests for this. Initially assigning the issue to him.

@jdespatis
Copy link
Contributor Author

@addyosmani @passy ok thanks !

For memory, the unit tests added by @SBoudrias for this.src are at: https://github.com/yeoman/generator/pull/378/files

So Inspiration has to be taken there in order to do the same thing for remote.src I guess

@sindresorhus
Copy link
Member

@passy ping, since you're assigned to this ;)

@coveralls
Copy link

Coverage Status

Coverage increased (+1.41%) when pulling 82b13cd on jdespatis:file-utils-for-remote into 248e469 on yeoman:master.

@SBoudrias
Copy link
Member

I'm not super convinced by the unit tests added.

I think it'd be more relevant to test that the bases path are setup correctly on the env. E.g.:

assert(  remote.src.fromBase('.'), 'remote/path/' );
assert(  remote.src.fromDestBase('.'), generator.destinationRoot() );

(Also, it'd be better to send PR without merge commit included)

@jdespatis
Copy link
Contributor Author

@SBoudrias I can try a rebase -i if you want

You prefer to fork this PR to add your corrections @SBoudrias ? or you can make your own PR and I close this one, no problem also

@SBoudrias
Copy link
Member

I'm taking care of it ATM.

@jdespatis
Copy link
Contributor Author

ok thanks @SBoudrias !

@passy
Copy link
Member

passy commented Dec 12, 2013

@SBoudrias Thanks!

@SBoudrias
Copy link
Member

Merged on master, thanks a lot!

@SBoudrias SBoudrias closed this Dec 12, 2013
@jdespatis
Copy link
Contributor Author

cool ;) thanks

@jdespatis jdespatis deleted the file-utils-for-remote branch December 12, 2013 21:03
@addyosmani
Copy link
Member

Great to see this land. Thanks!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants