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

Add RFC 3164 timestamp support #1

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

CodeAnimal
Copy link

Added support for RFC 3164 format timestamp. Added tests. Updated
Readme. Made some (unnecessary) code clean-ups.

Added support for RFC 3164 format timestamp. Added tests. Updated
Readme. Made some (unnecessary) code clean-ups.
@moll
Copy link
Owner

moll commented May 27, 2014

Hey!

Thank you for taking the time to do this! I'll review this as soon as possible, but would you mind doing a quick favor for me? Strip semicolons, please. :-) If you don't have the time to do this, I can do that before merging myself.

Thank you again!

@CodeAnimal
Copy link
Author

Hey that's no problem! I needed this myself, so thought I'd contribute.

I'm not sure why you want to strip the semicolons... they are a crucial part of the JavaScript language, making code more concise and readable (of course in my opinion), and it's good practise to use them.
You're very free to make that change yourself before merging :) I have ran the code through JSLint and corrected all reasonable errors in index.js.

I look forward to using this module in my projects, thanks for your efforts. 👍

@moll
Copy link
Owner

moll commented May 27, 2014

Okie-dokey, I'll tweak it before the merge. Thanks! :-)

Fortunately semicolons aren't necessary for JavaScript, nor do they affect conciseness (unless one crams multiple statements to one line), but, yeah, it's somewhat subjective.

Out of curiosity, if you don't mind me asking, what do you use SyslogProtocol.js for?

Cheers

@CodeAnimal
Copy link
Author

I think in some ways it is very much subjective. I think it is a vital part of the ECMAScript language specification, allowing some cool tricks. In this script, I agree it isn't strictly necessary, but I think it is a good practise to use them anyway.

But this is your repo, you can do with it what you want, and I respect that :)

I have a large project incorporating a Radius server, handing out IPs from IP pools to certain PPoE servers, and I am reading the syslogs live from them to be informed as quickly as possible when an IP has been freed up. It is an interesting project, and I am enjoying it very much - nearly entirely written in Node.js :)

@moll
Copy link
Owner

moll commented May 27, 2014

Cool @ RADIUS. It's been a while since I last ran a RADIUS server.

So, yeah, I prefer to write without semicolons and do it consistently everywhere. The rules for automatic semicolon insertion are rather precise and you can tweak Jshint to honor that as well, so all's fine. The less punctuation the better it looks for my eyes. ;-)

@CodeAnimal
Copy link
Author

Yeah it's going well :)

And fair enough on the semicolon front. Hope my PR will help yourself and/or others in the future :)

Remove semicolons in index.js and index_test.js.
@CodeAnimal
Copy link
Author

I have removed the semicolons for you. I hope you get time to take a look at this and merge sometime soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants