pbrisbin / bugsnag-haskell

Bugsnag error reporter for Haskell
10 stars 7 forks source link

Export `Bugsnag` prefixed type synonyms #78

Open parsonsmatt opened 1 year ago

parsonsmatt commented 1 year ago

I'm working on upgrading our application to the new bugsnag from bugsnag-haskell. I'm finding that the removal of the Bugsnag prefix from the types is a significant downgrade in the legibility of documentation and "one look comprehension". I'm working around this by defining a local module that defines type BugsnagSettings = Bugsnag.Settings, but this is reliant on GHC not 'seeing through' the type alias in error messages and Haddocks.

It would be nice to have these built in to Data.Bugsnag.

A Data.Bugsnag.Compat compatibility module with pattern synonyms would also be nice for making the migration easier, but I recognize that as a lot more work.

pbrisbin commented 1 year ago

a significant downgrade in the legibility

I don't necessarily disagree, but I considered it worth it to fully off-load the maintenance of all API types to the separate bugsnag-hs package. (And then I de-prefixed our own Settings type to make it consistent.) It seems like to maintain a prefixed type synonym and/or patterns for every single type would fully negate that lower maintenance benefit.

If you wanted to PR such a .Compat module, I would accept it. But at this time I'm not inclined to author or maintain it myself.

I'm interested in poking a bit at "legibility of documentation", specifically. Do you mean the bugsnag package Haddocks? I don't exactly follow why a Thing prefix on every item is helpful or necessary in docs that are entirely specific to the Thing.

As a workaround to at least recover legibility in your own code, you could always import qualified as Bugsnag and use (e.g.) Bugsnag.Settings everywhere. That's what we do for this and other packages that have this style.

parsonsmatt commented 1 year ago

I have a class

class (Monad m) => MonadBugsnag m where
  askBugsnagSettings :: m BugsnagSettings
  localBugsnagSettings :: (BugsnagSettings -> BugsnagSettings) -> m a -> m a

Even if I qualify the name to Bugsnag.Settings, the identifier in rendered Haddock will be merely Settings. I can click through in the docs, but it is irritating to need to follow a link to disambiguate a Settings name.

According to TAGS, there are 14 Settings types in our project's dependencies. This ruins tag based jump-to-definition, and it makes grep much less useful for analysis.

Exception clashes with Control.Exception.Exception, which is especially awkward when you need both terms in scope for working with Exception e => ... and Exception -> ... terms in the same module.

I understand that this is primarily conflict with the naming philosophy of bugsnag-hs, and not anything you have real control over.

pbrisbin commented 1 year ago

Thanks for that additional context, it makes sense. I forgot about Exception, that one has been a pain-point for me too.

I understand that this is primarily conflict with the naming philosophy of bugsnag-hs, and not anything you have real control over.

Yeah, that's the core conflict. It's either we accept it, or we retake maintenance of the entire API (either as our own types or through synonym mechanisms).

Tangential, but we've done a similar thing with:

class HasBugsnagSettings env where
  bugsnagSettingsL :: Lens' env Bugsnag.Settings

Then we can recover what you're using MonadBugsnag for in any MonadReader env m, HasBugsnagSettings env:

This Has-class and Lens-style usage is something we like and I'm considering bringing into the library. Do you have any thoughts on that?

parsonsmatt commented 1 year ago

I generally prefer MonadFoo (m :: Type -> Type) class over HasFoo (a :: Type) as the primary interface, but it's really easy to provide both and use DerivingVia -

newtype ViaReader m a = ViaReader (m a)

instance (HasBugsnagSettings r, MonadReader r m) => MonadBugsnag (ViaReader m)

newtype App a = App (ReaderT Env IO a)
  deriving MonadBugsnag App via ViaReader App

instance HasBugsnagSettings Env

-- allowed
newtype BugsnagChanger a = BugsnagChanger (StateT BugsnagSettings IO a)

instance MonadBugsnag BugsnagChanger where
  askBugsnagSettings = get
  localBugsnagSettings f action = do
    prev <- get
    put (f prev)
    a <- action
    put prev
    pure a

instance MonadBugsnag IO where
  -- get stuff from environment variables i guess? idk

If you rely on MonadReader, then it'll also stop at the first ReaderT. Which means that, ie, SqlPersistT ~ ReaderT SqlBackend won't ever be able to use (MonadReader env m, HasBugsnagSettings env) functions, since it'll say No instance for HasBugsnagSettings SqlBackend. You'd really prefer to be able to say instance (MonadBugsnag m) => MonadBugsnag (ReaderT r m) and delegate to an underlying instance, which would work with SqlPersistT (BugsnagT IO).

(this is actually a big design wart in persistent that i'd love to fix, too! both the type SqlPersistT = ReaderT and that everything else is in MonadReader SqlBackend m or similar)