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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support dropping folders #17

Merged
merged 1 commit into from Oct 4, 2019

Conversation

kevinleedrum
Copy link
Contributor

Happy Hacktoberfest! 馃巸

This PR adds the ability to drop folders, which will be parsed recursively for files.

Before: 鈽癸笍

oDMMQDjHOR

After: 馃槃

e64MB8MUhg

@safrazik
Copy link
Owner

safrazik commented Oct 3, 2019

Thank you for the pull request. This is a very welcomed feature (that I had thought of supporting in the future). I will do some testing on my side and merge. Thanks. Happy Hacktoberfest!

@safrazik
Copy link
Owner

safrazik commented Oct 4, 2019

Hi @kevinleedrum great work. I tested it and works perfectly. My only concern is that you are using async/await, which will result in the regeneratorRuntime being added to the dist files which in turn affect the dist file size (+84KB unminified, +25KB minified) . I have avoided using some es6+ features on purpose because I was too concerned about the file size. As this is a library, I believe that we should keep a minimal footprint for consumption from any development setup.
Will you be able to update your pull request? Or I can merge and update it to plain promise based implementation before the next release.

@kevinleedrum
Copy link
Contributor Author

Good point. I will convert it to promises.

@kevinleedrum
Copy link
Contributor Author

@safrazik I've replaced async/await with promises, and I also replaced const and let with var to match the rest of the project.

@safrazik safrazik merged commit 8466557 into safrazik:master Oct 4, 2019
@safrazik
Copy link
Owner

safrazik commented Oct 4, 2019

@kevinleedrum Thank you for your work.
BTW, I don't think const or let will cause any trouble, but yeah, it's good to keep things consistent. Since the library evolved by time (before being open sourced) you will see a lot of old fashioned code. It would be better to do some code refactor in the future. Thanks for helping.

@Garito
Copy link

Garito commented Oct 4, 2019

My two cents on that:
const, let & var have different repercussions (in that case scope & performance)
const should be used for non mutable values for performance
let & var should be used to control the scope of the variable (var being less "local")

So, I would rather prefer that kind of consistence

@safrazik
Copy link
Owner

safrazik commented Oct 4, 2019

Sure. I haven't seen a better (short) explanation than this for const and let. Thanks.

@safrazik safrazik added this to the 1.3 Rock Wren milestone Oct 8, 2019
@safrazik
Copy link
Owner

safrazik commented Oct 9, 2019

@kevinleedrum Support for dropping folders landed in version 1.3 Rock Wren. Thanks for your contribution.

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

Successfully merging this pull request may close these issues.

None yet

3 participants