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

split off DiagnosticReport + reporters into their own library? #25

Closed
Tracked by #45
zkat opened this issue Aug 21, 2021 · 17 comments · Fixed by #56
Closed
Tracked by #45

split off DiagnosticReport + reporters into their own library? #25

zkat opened this issue Aug 21, 2021 · 17 comments · Fixed by #56

Comments

@zkat
Copy link
Owner

zkat commented Aug 21, 2021

No rush, I think, because the intention is very much that anyone using miette will also probably be using the reporter, but I'm hoping people write their own bespoke reporters against the "protocol".

...maybe that means we need miette-protocol, and miette is the Big Chonker library? That would let us do this refactor without being semver-breaking. 🤔

@zkat
Copy link
Owner Author

zkat commented Aug 22, 2021

lol apparently miette went from 18kb to 118kb when I added the reporter. And I think that's on top of the extra deps 😩

@fogti
Copy link

fogti commented Sep 1, 2021

What would miette-protocol contain in that scenario? I suppose many libraries would just use miette to derive custom error data structures, but won't report/print/format any errors themselves, so I guess that the protocol would include that derive stuff as well? Or should that be split into yet another crate?

@zkat
Copy link
Owner Author

zkat commented Sep 1, 2021

Yes, that's the idea: miette-protocol only for the traits and the derive macrology, and the main miette crate with the reporter and all the extra dependencies it brings in (which is... a number of them).

That said, the 118kb was actually because I was including images in the package--oops! I'm not too fussed about it for now.

I did give this a shot and it turned out to be Very Hard, Actually, so I just gave up. I couldn't find a way to export the derive macro both from miette and miette-protocol without having users always depend on miette-protocol itself. :( I think I would need two copies of miette-derive, one for each of the two libraries, in order to make this work.

@fogti
Copy link

fogti commented Sep 1, 2021

I think tracing (as an example of a roughly similar crate) has a separate tracing-subscriber crate which does the formatting/display/logging stuff. Applications are required to include both in their dependencies. I don't think that that's too much of a hassle, especially when the crate containing the protocol is pretty stable.

@zkat
Copy link
Owner Author

zkat commented Sep 10, 2021

I think I'm just not gonna do this for the foreseeable future. The crate isn't that big anyway and there's been no asks for this to be a "minimal" crate of any sort.

@zkat zkat closed this as completed Sep 10, 2021
@olson-sean-k
Copy link
Contributor

olson-sean-k commented Sep 15, 2021

Just wanted to drop in and mention that I think this would be a great idea moving forward. I'm wrestling with adding miette as an optional dependency, but it's a bit challenging as diagnostics tend to be cross-cutting and conditional compilation gets gnarly fairly quickly. I'd like to avoid requiring unused terminal dependencies (e.g., atty, owo-colors, textwrap, term_size and unicode-width) when downstream users aren't concerned with pretty errors or terminal output.

That being said, I realize that this could be a somewhat difficult and disruptive change, as it would split miette into at least one new additional crate. FWIW, I think that would be manageable for miette users and could even become natural if alternative reporters are supported in the future (perhaps related to #50?).

(Thanks for all of the great work!)

@zkat
Copy link
Owner Author

zkat commented Sep 16, 2021

Reopening this because it's been bothering me again, and I think you're totally right. Especially now that I've picked up even more dependencies while styling the reports nicely.

@zkat zkat reopened this Sep 16, 2021
@zkat zkat mentioned this issue Sep 16, 2021
14 tasks
@zkat
Copy link
Owner Author

zkat commented Sep 16, 2021

@olson-sean-k what do you think of the idea of having all the fancy reporter stuff be optional, behind a single feature flag that's enabled by default?

@olson-sean-k
Copy link
Contributor

what do you think of the idea of having all the fancy reporter stuff be optional, behind a single feature flag that's enabled by default?

Yeah, I think that would work well enough, though it would require a bit more diligence from users since it only takes one dependency specification that forgets to disable default features for such a feature to activate in a build.

I think separating these concerns at crate boundaries is the better long-term solution, but I'm guessing that adding a feature would be much quicker and easier to implement. Maybe the feature could be a transitional thing? Then any crate decomposition can be handled later (or abandoned if it turns out it just won't work as well as I had hoped).

zkat added a commit that referenced this issue Sep 16, 2021
BREAKING CHANGE: The default fancy reporter is no longer available unless you enable the "fancy" feature. This also means you will not be pulling in a bunch of deps if you are using miette for a library

Fixes: #25
@zkat
Copy link
Owner Author

zkat commented Sep 16, 2021

I've gone the feature route (see bc495e6) and I'm overall pretty happy with it. I've also made the feature disabled by default.

@zkat
Copy link
Owner Author

zkat commented Sep 16, 2021

note: I also tried, again, to split this off into a separate crate, and it really didn't work very well because of a combination of circular dependency stuff and the fact that the derive macro specifically needs a crate called miette to be available. So I think this is what we're gonna settle on long-term, and honestly it works pretty well?

@zkat zkat linked a pull request Sep 16, 2021 that will close this issue
@olson-sean-k
Copy link
Contributor

Awesome! Thanks for putting this together. I like making this an opt-in rather than an opt-out feature too. I think this solves the underlying issue I described (without needing to carve things up, no less). 🎉

Once the feature change lands I'd consider the dependency issue closed, but I hope you don't mind if we continue to discuss spinning off crates a bit. 😄 I'm generally a fan of that approach, though I admit that it probably wouldn't be the most immediately useful or practical change. Please feel free to ignore everything below!

I also tried, again, to split this off into a separate crate, and it really didn't work very well because of a combination of circular dependency stuff and the fact that the derive macro specifically needs a crate called miette to be available.

I see. Perhaps miette could be more of a "meta-crate" that composes miette-protocol (or miette-core or whatever) and miette-derive? Then users would end up depending on miette and another crate for reporting (perhaps the default being miette-report or something). The meta-crate could even provide a feature to pull in the default reporting crate as well and import its items into its modules.

As individual dependencies:

[dependencies]
miette = "^3.0.0" # Composes `miette-core` and `miette-derive`.
miette-report = "^1.0.0" # Provides a reporter and error type definitions.
use miette::Diagnostic;
use miette_report::Result;

#[derive(Diagnostic)]
#[diagnostic(...)]
pub struct Error { ... }

fn main() -> Result<()> { ... }

Using a feature that composes all of the first-party crates (a bit like the incoming feature change):

[dependencies.miette]
version = "^3.0.0"
features = ["report"] # Composes `miette-core`, `miette-derive`, and `miette-report`.
// `miette_report` items are re-exported inside `miette`.
use miette::{Diagnostic, Result};
...

With that arrangement, users would never interact directly with miette-core nor miette-derive; they'd just take a dependency on miette and let it pull everything together (and miette-derive would always have access to a miette crate that re-exports the necessary miette-core types). Authors of reporting crates would only require miette-core. Maybe something like this could work.

@zkat
Copy link
Owner Author

zkat commented Sep 16, 2021

I mean that might work, but I'm not sure what the ultimate benefit would be. I do like small crates, but miette, without all those console dependencies, is actually pretty tiny?

@olson-sean-k
Copy link
Contributor

Considering that miette is fairly small, I think the best I could say about spinning off crates is that it would discourage tight coupling and move all reporter implementations to discrete crates (avoiding any quirks or asymmetry between miette's implementation and others).

Yeah, that's kinda vague and not terribly compelling. Feature gating the reporter is probably all that's needed here. 👍🏽 Sorry for the noise!

@zkat zkat closed this as completed in #56 Sep 22, 2021
zkat added a commit that referenced this issue Sep 22, 2021
BREAKING CHANGE: The default fancy reporter is no longer available unless you enable the "fancy" feature. This also means you will not be pulling in a bunch of deps if you are using miette for a library

Fixes: #25
@olson-sean-k
Copy link
Contributor

I think I missed something important here that I've run into lately: fundamental traits like Diagnostic (the protocol) probably won't change as often as the code written atop them. Each breaking release of miette requires that intermediate crates that merely support the miette protocol must upgrade and publish prior to their dependents. This problem gets worse if a crate depends on more than one crate that supports the miette protocol.

From a quick glance at time of writing, it looks like Diagnostic, MietteError, SourceCode, SourceSpan, SpanContents, and SourceOffset all currently participate in the protocol (changes to any of these items are protocol changes). The change log does not seem to consider all of these items when announcing a protocol change, but using it as a very rough heuristic suggests that version gaps as large as 3.0.0 to 4.6.0 don't feature protocol changes. Do any of the maintainers have a guess for how often these items are changed between breaking releases?

A miette-protocol crate could reduce the burden on intermediate crates if it turns out that protocol changes don't happen (or don't need to happen) frequently. Changes confined to the miette crate would not require protocol-only crates to upgrade prior to their dependents so long as the protocol has not changed. Instead, intermediate crates only need to be concerned with breaking releases of miette-protocol. I'm honestly not sure how many intermediate crates like this exist in the ecosystem today though.

AFAICT, breaking releases that have not changed the protocol have been published in the recent past, so perhaps this is worth revisiting. Any thoughts? Thanks!

@zkat
Copy link
Owner Author

zkat commented Jul 14, 2022

I mean, my answer is pretty much the same as I gave in #25 (comment), which is that everything worked out kinda janky when I tried to spin it off into its own crate.

In general, I've been very conservative about breaking changes to the core protocol, but I think it's also worth noting that, most of the time, people will use the protocol through miette-derive. That is also fairly stable.

@fogti
Copy link

fogti commented Jul 15, 2022

For the derive macro stuff, many derive macros provide a way to simply override the "backend" crate name.

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 a pull request may close this issue.

3 participants