Skip to content

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

@kevinleedrum kevinleedrum force-pushed the parse-dropped-folders branch from fda25fe to 6eb0304 Compare October 2, 2019 19:28
@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 kevinleedrum force-pushed the parse-dropped-folders branch from 6eb0304 to f0f0585 Compare October 4, 2019 12:48
@kevinleedrum kevinleedrum force-pushed the parse-dropped-folders branch from f0f0585 to f3e716c Compare October 4, 2019 12:51
@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.

3 participants