Mesabloo / diagnose

A simple library for reporting compiler/interpreter errors
https://hackage.haskell.org/package/diagnose
BSD 3-Clause "New" or "Revised" License
258 stars 19 forks source link

Add instances for Error.Diagnose.Style #17

Closed spacekitteh closed 1 year ago

Mesabloo commented 1 year ago

Hello, thanks for the PR. I have one question though: what are these needed for? For example, I don't see the point of neither Show (because these annotations are never meant to be seen) nor Ord (an order does not make sense on such datatype). Also, what would be Generic useful for? Furthermore, would it be possible not to depend on GHC's internals (the GHC.Generics import) in favor of a more "standard" import which may be available in other compilers?

spacekitteh commented 1 year ago

They are useful if you want to embed marked-up, laid-out diagnostics into other structures which require those instances. As for Generic, it's useful for, e.g., deriving Hashable.

Furthermore, would it be possible not to depend on GHC's internals (the GHC.Generics import) in favor of a more "standard" import which may be available in other compilers?

Unfortunately I don't believe there is a standard import for this; and unfortunately I'm not aware of any other compiler even implementing Generics :(

spacekitteh commented 1 year ago

Although it could be made to be conditional on whether it's compiling with GHC or not I guess?

Mesabloo commented 1 year ago

This definitely could be done with CPP, if I remember correctly. Although GHC is the most prominent implementation of a Haskell compiler, I'd rather try not to lock users into using only GHC if this can be avoided. I'd need to check the remaining of the code though...which should be changed (mostly) in the near future (I hope), so maybe I'll check later.

spacekitteh commented 1 year ago

I have a few more improvements I'd like to do, so I could roll this PR and the CPP stuff into a future PR if that's preferable?

Mesabloo commented 1 year ago

There's no problem in doing this in here, as long as the PR is not as massive as #14 (which is to be merged soon enough). To be honest, I wouldn't spend much time on the current code (unless specific additions -- like instances -- are really needed), as it should be changing drastically.

spacekitteh commented 1 year ago

A'ight, I'll wait :) In the meantime, what do you want me to do with the Generics?

Mesabloo commented 1 year ago

If this is very much needed right now, I can try to merge this and release a new version in the day. I don't really know how long it will take for #14 to be merged; I don't want to hold back any update while waiting for it.

spacekitteh commented 1 year ago

Eh, I don't mind having a few orphan instances for a while; it looks like I'll have to overhaul all my diagnostics-related code soon anyway :)

Mesabloo commented 1 year ago

Changes related to the core of the library (creating reports, diagnostics and positions) should be minimal, as we tried to keep as much as existed before. Most of it should just fall under renaming constructors or types, which shouldn't be much of an issue (we'll release an entirely new version anyway, to indicate breaking changes, but we may also add backwards-compatible constructors and type aliases to facilitate the transition, and deprecate them straight away to encourage changes). Where much of it is going to change is the actual rendering step, which will be separated into separate package depending on the style of diagnostics you want. If you'd like to, we also provide a small API (with common basic tasks like annotating a source line for markers) to facilitate creating new layouts, although this still isn't an easy task (for obvious reasons; all the rendering logic has to be described there). All of the layouts must obey to a common interface (providing a Layout and a Style for the inner annotations), which makes them suitable for being rendered/pretty printed to a Doc AnsiAnnotation. As of now, there are 4 styles of layouts :

Mesabloo commented 1 year ago

Closing this as it should have been included in #22.