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

The ext package #51

Open
GeertJohan opened this issue Mar 5, 2015 · 7 comments
Open

The ext package #51

GeertJohan opened this issue Mar 5, 2015 · 7 comments

Comments

@GeertJohan
Copy link

In #43 @inconshreveable wrote:

A separate conversation should probably go into whether the ext package even makes sense and should continue to exist in v3.

Until recently I thought ext was just a folder containing packages for external logging systems, as that is mentioned in #5. I think that does make sense to avoid an eventually huge number of logging-service related packages in the project root.

However, for extra handlers such as the ext.EscalateErrHandler and ext.HotSwapHandler, I think normal sub-packages would be better. And I think extra Handlers are only required to be in a sub-package when one or more of the following is true:

  • depends on a third-party package
  • compiles with cgo
  • imports unsafe (?)
  • a large amounts of background work being started on init()

Otherwise, they might just as well be part of log15 itself, right?

As an example; the HotSwapHandler could go into gopkg.in/inconshreveable/log15.v2/hotswap. According to http://blog.golang.org/package-names that could look something like this:

type HotSwapHandler struct {
    h log15.Handler
}

func New(h log15.Handler) *HotSwapHandler { … }
func (h *HotSwapHandler) Log(r *log15.Record) error { … }
func (h *HotSwapHandler) Swap(h log15.Handler) { … }

And usage:

import "gopkg.in/inconshreveable/log15.v2/hotswap"

hotswapHandler := hotswap.New(fooHandler)
// etc

// later
hotswapHandler.Swap(barHandler)

So yes, I think ext should continue to exist, but only as folder for logging to external services. As described in #5.

@ChrisHines
Copy link
Collaborator

I agree with your criteria for when to include code in the main log15 package.

Perhaps this is a different topic, but it is mildly annoying that one must import log15.v2 in order to implement a Handler (because of the *log15.Record argument). So Handlers are always coupled to log15.v2. This may cause problems down the road for people that implement custom handlers in a library. Consumers of such Handlers will have a harder time upgrading to a putative log15.v3 because I'm pretty sure that all the Handlers in an application must have the same import path for *Record. I'm not sure what to do about that yet.

@GeertJohan
Copy link
Author

@ChrisHines Excelent point! I hope you don't mind I've opened a new issue for it.

@inconshreveable What do you think about ext ?

@ChrisHines
Copy link
Collaborator

I'm not sure about the restriction against unsafe. Log15 already imports unsafe except on appengine.

@GeertJohan
Copy link
Author

Yeah me neither.. That's why I had the question mark behind it.
Well, now that you say it: appengine could be a valid reason to have unsafe-dependent handlers have their own package. We wouldn't want log15 to become unusable on appengine so it would have to be added with negative build tags. And maybe it makes more sense to say "packages x y and z are not available on appengine" instead of "Handlers x y and z are not available on appengine"?
What do you think?

@GeertJohan
Copy link
Author

Update on this? Implement it in v3 branch?

@ChrisHines
Copy link
Collaborator

We can't remove any existing exported identifiers from packages without an API version bump, so I don't think we want to do this until we have a more compelling reason to create log15.v3.

@dsoprea
Copy link

dsoprea commented Aug 1, 2016

I've only just skimmed over the bottom of this conversation since I'm tight on time, but, speaking of not wanting to be "unusable on appengine", it seems like we're using "unsafe" and I'm trying to use log15 on AppEngine. Is there a fix for this or am I out of luck with log15?

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

No branches or pull requests

3 participants