search

lord / wargo

Easy Rust to WebAssembly
MIT License
261 stars 9 forks source link

Better os checking #12

Closed b-m-f closed 7 years ago

b-m-f commented 7 years ago

Have removed eslint and prettier stuff. Still a new file. I think this is better for maintainability. Otherwise the setup file is huge after a while.

b-m-f commented 7 years ago

Oh btw. I would really add eslint and prettier. If you want I can make a separate MR for it.

That will catch errors during typing and make Code reviews much easier, because white space stuff, quoting etc. is all handled.

F.e. I have not caught that child_process was renamed to childProcess when cherry-picking my previous commit. With eslint I would have seen the error right away, as the variable would not have been used :)

b-m-f commented 7 years ago

I have further modularized the dependency checking. Just tested on MacOs and it works fine. Cant test Ubuntu though.

b-m-f commented 7 years ago

Heyho. Whats up with this? I wont be able to maintain this MR indefinitely :)

lord commented 7 years ago

Hey, sorry for the delay — I think it's an important feature to have, but like I mentioned in my comment before, I think I'd rather have a simpler solution in just the main module. This PR still has a bunch of style changes adding semicolons that aren't relevant to the change, it still is lowercasing the OS name, and it still has bunch of extra functions. Rather than make you keep changing it to appease my arbitrary coding standards, I've got a smaller change here that I think does roughly the same thing. Let me know if you think this solves your problem, or if you think there are issues not addressed by that diff?

b-m-f commented 7 years ago

LGTM :) Just thought that this might receive some more platform specific tickets and should be easier to extend.

But wow. I did not even notice the semicolon stuff :D sry about that

lord commented 7 years ago

:+1: no worries! I've merged the platform code into master.