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

Fixed issue #38 (added more sophisticated code optimization) #39

Merged
merged 4 commits into from May 8, 2017

Conversation

T-vK
Copy link
Contributor

@T-vK T-vK commented May 7, 2017

It's now able to successfully remove multi-line comments and it also gets rid of unnecessary new line characters and more.
I uploaded a few test files and they ran as expected. I also downloaded them again to confirm that it was working as expected.

It also minifies variable names btw. I was a bit worried about that at first, but it's only happening to variables that are specifically defined using local, so everything fine.

@AndiDittrich
Copy link
Owner

Dear T-vK,

thanks for your contribution. i think we have to change some details of your pull request first.

generally, i prefer to add a custom option/flag for this kind of optimization/minification because it could may break existing projects (see Semantic Versioning).

my suggestion is

  1. to introduce different optimization levels (like gcc) - maybe --optimize size (size only, classic whitespace removing) --optimize minify (AST based minification)

  2. or add a dedicated --minify option for luamin optimization

i would prefer option 1 - what do you think ?

best regards, Andi

@T-vK
Copy link
Contributor Author

T-vK commented May 7, 2017

How about this:
We add a deprecation message to the --optimize flag, but leave it the way it was.
Then we add a --minify flag which uses luamin.

I don't think we would need more optimization levels. luamin was the only module I could find and I looked at all 176 search results for lua.
Besides that we would have to allow the optimize property in a .nodemcutool file to be either true, false or a string. Certainly possible, but kind of weird. And it could potentially (although unlikely) break other tools that try to parse that file.

What do you think?

@AndiDittrich
Copy link
Owner

this solution sounds promising :)

could you please modify your pull request/add a new file LuaMinifier.js to NodeMCU-Tool which invokes luamin ?
i will add the deprecate notice soon

@T-vK
Copy link
Contributor Author

T-vK commented May 8, 2017

I implemented it. But I think I found another bug #40. Basically --optimize doesn't work unless you put it in the .nodemcutool. Since I implemented --minify the exact same way, the same applies to --minify now.
Both commands work fine when using them in .nodemcutool.

@AndiDittrich AndiDittrich merged commit b4e4da7 into AndiDittrich:master May 8, 2017
@AndiDittrich
Copy link
Owner

v2.2.0 is out including your pull-request

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

2 participants