Closed 0x7f closed 8 years ago
Looks nice, but to be honest, I don't understand your code since I am not familiar with C++ neither the win32 native API.
Can I add you as a contributor/maintainer to this repo and the npm package so you can push directly?
Done deal :)
Hehe, that was unexpected, but why not ;) Will clean up the commit again and let you know when I'm ready to merge so you can have a final look again.
Finally found a minute to clean up the commit a bit (improve error messages using GetLastError()
and ensure formatting & naming is consistent). What do you think about the current state?
A thought about the external API: I'd like to make the .log()
call async by default (in my experience WIN32 can block for ages sometimes depending on what they do) and add an additional .logSync()
call if logging synchronously is required. But did not want to introduce this breaking change in this Pull Request already as it also requires updating dependent libraries like winston-winlog
.
Whats your opinion about the breaking change?
You can do it in this PR and release it as a major, no problem El El vie, 26 de feb de 2016 a las 6:55 a.m., Maximilian Haupt < notifications@github.com> escribió:
A thought about the external API: I'd like to make the .log() call async by default (in my experience WIN32 can block for ages sometimes depending on what they do) and add an additional .logSync() call if logging synchronously is required. But did not want to introduce this breaking change in this Pull Request already as it also requires updating dependent libraries like winston-winlog. Whats your opinion about the breaking change?
— Reply to this email directly or view it on GitHub https://github.com/jfromaniello/node-windows-eventlog/pull/11#issuecomment-189199976 .
Finally found a minute to introduce the async log function as well. Also bumped major version to 2.0 as discussed and moved some functionality to free function so they can be used by the EventLogAsync
worker as well.
To be honest, I do not really know how the pre-built node modules work. Do you want to keep them? If so, could you please provide them, because I get a Security Exception on my machine when I try to run the powershell script.
Good work!
I think we should remove the prebuilt stuff, is not a good practice to submit binaries to npm and most win devs are used to install the compiler on windows.
Alright, will remove them.
Done. Will squash the commits now to prepare them for merging.
Two things came to my mind that should be done before merging:
What do you think?
@0x7f yes, I agree
@jfromaniello I updated the README and made the severity optional. Whats your opinion about the current state? Is it mergable?
@0x7f looks wonderful!
Imho, using the win32 native API is more reliable than using clr/.Net in node bindings. That's why I ported the code to C++ and Nan (to get it running under latest node.js again).