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 SyslogLogger #294

Merged
merged 1 commit into from
Aug 24, 2013
Merged

Add SyslogLogger #294

merged 1 commit into from
Aug 24, 2013

Conversation

jkm
Copy link
Contributor

@jkm jkm commented Aug 22, 2013

The SyslogLogger logs in syslog format according to RFC 5424.

It fixes issue #283. Please review and tell me if it's good to go.

@@ -407,6 +407,195 @@ class HTMLLogger : Logger {
}
}

/// Facilities
enum Facility
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would call this 'SyslogFacility', or, better, move it into the 'SyslogLogger' class. It will be confusing for someone looking at the documentation to see these types at the global scope, left wondering for what they are used.

In fact, I would move all definitions from here up to the SyslogLogger class into the class so that it is nicely self-contained, possibly making them all except Facility private as it is always easier to add public symbols that it is to remove them (WRT backwards compatibility).

@s-ludwig
Copy link
Member

Thanks! This looks good to go apart from the mentioned minor issues.

There is one additional issue related to the DDOC comments, and most people don't know about it, because for DMDs documentation generator it usually doesn't make a difference: The first paragraph of a doc comment is treated as a summary (in DDOX documentation this is used in the overview tables, see also the ddoc documentation under "Sections"). Thus, it should usually just be a single short sentence describing the complete functionality.

@jkm
Copy link
Contributor Author

jkm commented Aug 23, 2013

Many Thanks. Comments are very right.

The SyslogLogger logs in syslog format according to RFC 5424.
@jkm
Copy link
Contributor Author

jkm commented Aug 23, 2013

Sorry for destroying the old commit. Didn't know a better way. I want this to be in one commit. I addressed all of the issues mentioned. Please have a look though. Thanks.
I'm a bit disappointed about D's enum for an AA. For me enum was a synonym for at compile time. This does not seem to be true. I can only remember a small number of exceptions to a rule. Anyway.

@s-ludwig
Copy link
Member

Looking good, thanks a lot. No worries about the deleted commit, I don't think there is another (practical) way. Merging in now...

Yeah, AAs in general are one of the major weak spots of D right now. I hope one day someone will manage to make a complete, clean reimplementation. And enum sometimes indeed is a strange beast, as it does require the expression to be compile-time executable, but for (non-immutable) reference types it still does (and has to) create dynamic instances, or the manifest constant nature of the enum wouldn't hold because the returned reference can be mutated. A possible fix for that would be to make enum imply immutable, but that would in turn break a lot of use cases.

s-ludwig added a commit that referenced this pull request Aug 24, 2013
@s-ludwig s-ludwig merged commit 72a86d8 into vibe-d:master Aug 24, 2013
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.

3 participants