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

Update IRC appender to v3 appender style #65

Merged
merged 12 commits into from
Jul 3, 2014
Merged

Update IRC appender to v3 appender style #65

merged 12 commits into from
Jul 3, 2014

Conversation

crisptrutski
Copy link
Contributor

Refers to: #41

Includes some small incidental fixes/tweaks, but maintained backwards compatibility I believe.

Non-backwards compatible changes that present themselves:

  1. Replace global conn atom with one scoped/created inside make-irc-appender.
  2. Replace prefix-fn pattern with fmt-output-fn usage

Also happy to add some tests (guess I'd just mock the irclj functions?)

@ptaoussanis
Copy link
Member

Hey Chris, this looks great - thank you! Sorry for the delayed reply btw, have been travelling.

Replace global conn atom with one scoped/created inside make-irc-appender.

Sound good to me. I get the impression that the current global conn is effectively private, so don't think this should be breaking in most cases.

Replace prefix-fn pattern with fmt-output-fn usage

I think that'd be an acceptable break, but thanks for checking.

Also happy to add some tests (guess I'd just mock the irclj functions?)

If you think there's something specifically worth automated testing, otherwise wouldn't worry about it.

So I'll wait on an updated PR with the additional 2 changes? Again, much appreciated! Cheers :-)

@crisptrutski
Copy link
Contributor Author

No worries on delay - how were the travels?

Took a stab at those extra changes, and left just a smidgeon of testability via comments.

RE: the prefix-fn business, seemed like that function was never being called, at least in my testing. Should behave along lines of original intention now (level is wrapped in [..] though). Using a fmt-output-fn option in the actual appender config to leave this configurable, not sure if that was the appropriate pattern.

Now that you're back at the helm I'll be on the look out for more places to sneak my code into your high profile libraries ;)

@crisptrutski
Copy link
Contributor Author

Also let me know if you want these commits squashed up a bit, err'd on the side of micro incase you wanted any changes knocked out.

@ptaoussanis
Copy link
Member

No worries on delay - how were the travels?

Oh, they were still ongoing :-) Back now, really sorry about the huge delay getting this merged - was trying to actually take a real work-free vacation for once, so no non-critical work for a few weeks :-)

Now that you're back at the helm I'll be on the look out for more places to sneak my code into your high profile libraries ;)

Sure, always happy to see good PRs like these! (I'll be a lot more responsive now btw).

Also let me know if you want these commits squashed up a bit, err'd on the side of micro incase you wanted any changes knocked out.

No, no - this is perfect, can squash from my end if I like. Merging now.

Cheers and, again, thanks!

ptaoussanis added a commit that referenced this pull request Jul 3, 2014
@ptaoussanis ptaoussanis merged commit c2059e6 into taoensso:master Jul 3, 2014
@crisptrutski crisptrutski deleted the cleanup-v2-appenders branch July 11, 2014 16:46
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.

None yet

2 participants