Get priority levels from their preprocessor definitions.

jue89 / node-systemd-journald

Native bindings to journald

MIT License

28 stars 10 forks source link

Get priority levels from their preprocessor definitions. #4

Closed jez9999 closed 8 years ago

jez9999 commented 8 years ago

The PRIORITY levels should really come from the definitions in syslog.h - this implementation gets them there instead of guessing at them from the JavaScript.

jue89 commented 8 years ago

Thank you for your PR!

ATM I'm too busy for reviewing larger PRs. I will find time in the next few days to have a deeper look into your PR. Please be a little bit patient ;)

jez9999 commented 8 years ago

Actually reading through the code again, that "+ 9" is a magic number. We should probably do:

#define PRIORITYPREFIX PRIORITY=
#define PRIORITYPREFIXLEN 9

... then later on:

size_t strLength = strlen(priorityStr) + PRIORITYPREFIXLEN;

...

snprintf((char*) iov->iov_base, strLength + 1, "%s%s", PRIORITYPREFIX, priorityStr);

jue89 commented 8 years ago

I'm going to merge your PR tonight and will keep that in mind. Afterwards I let you have a look onto my changes before I merge everything back to master.

jue89 commented 8 years ago

I just pushed a temporary branch that includes your PR and my changes: https://github.com/jue89/node-systemd-journald/tree/feature/prio-levels-from-syslog

Beside removing the magic number "+ 9" I also changed the code a little bit. The major changes are:

Removing the multiply by 10 ...
... and thus I was able to implement a look-up instead of the switch/case construct. In my opinion this is a little bit cleaner. I hope you are okay with this change.
Check all arguments in terms of type and existance.
Also cleaned the unit tests a little bit.

What are your thoughts about my changes?

jez9999 commented 8 years ago

Hi,

It mostly looks good to me! I have left a few comments on the commit itself, though. Tell me whether you can seem them or not.

jue89 commented 8 years ago

Found your comments! Thank you very much! I'm going to fix the last things and then merge the branch to master tonight.