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

ES6 refactor #360

Merged
merged 17 commits into from Feb 18, 2017
Merged

ES6 refactor #360

merged 17 commits into from Feb 18, 2017

Conversation

JPeer264
Copy link
Collaborator

@JPeer264 JPeer264 commented Feb 17, 2017

(#355 ) I did some more refactoring. Now everything except move/index, copy/ncp and remove/rimraf are as much as possible refactored to ES6. I wasn't sure on the skipped files, as they are from another library or got a comment with something like needs rewrite or similar.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 85.138% when pulling 6eff3e5 on JPeer264:feature/refactor-es6 into 082342e on jprichardson:master.

Copy link
Collaborator

@RyanZim RyanZim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, noticed one old comment that could be removed.

var path = require('path')
'use strict'

const path = require('path')
// path.isAbsolute shim for Node.js 0.10 support
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort of unrelated, but this comment should have been removed earlier, wouldn't hurt to remove it here.

@RyanZim
Copy link
Collaborator

RyanZim commented Feb 18, 2017

I'm fine with doing the skipped files, all of them are already significantly modified from their original state.

If we decide to make changes in #292, you could skip ncp; not sure what we're doing there yet.

@JPeer264
Copy link
Collaborator Author

JPeer264 commented Feb 18, 2017

@RyanZim I am on move/index right now. As there are many functions inside I would suggest to outsource them into own files and require them separately into index.js. How you think about it?

EDIT:
This would also be a first step for a cleanup, regarding to the comment on the top of the file

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 85.138% when pulling 7471352 on JPeer264:feature/refactor-es6 into 082342e on jprichardson:master.

@RyanZim
Copy link
Collaborator

RyanZim commented Feb 18, 2017

move/index is only 158 lines, I wouldn't split it personally, perhaps @jprichardson differs.

BTW, don't bother with copy/ncp; I'm working on that right now.

@manidlou
Copy link
Collaborator

@RyanZim is this ready to merge?

@RyanZim RyanZim merged commit f831b05 into jprichardson:master Feb 18, 2017
@RyanZim
Copy link
Collaborator

RyanZim commented Feb 18, 2017

Merged. @JPeer264 Thanks again for all your work here.

This was referenced Feb 20, 2017
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.

None yet

5 participants