zesterer
Report kind moved to trait
this is a first draft of the ideas mentioned in #135 its very backwards compatible but does not solve the 2 other issues. If we want to solve those issues its probably best to move msg itself INTO the trait so we have ReportText which is an iterator over text+color.
obviously that change is going to completely break all of the old API so I haven't added it yet
@zesterer 2 and 3. are incredibly nice ideas gonna add them as well as your recommended fixes
I am kinda worried about removing custom right away because it breaks old code. I do think its probably wise to deprecate it eventually, but let's give people deprecation warnings first for a version or 2.
in the meantime if you're not using custom, then this 1 line
pub type ReportKind = ariadne::ReportKind<'static>;
solves the lifetime issue entirely. Although I do see the issue that this isn't obvious to newer Rust devs. So, potentially adding this exact example code to the ReportKind docs is a good idea.
It's also probably easier to use String or (Color,String), or something like what #135 did and actually have ErrorStyle as a ZST we just let you have (probably should mention them as a co-author).
its also important to consider if get_color makes sense when we do multi-color. I think it would basically just be a config option that the multi-color selector defaults to caring about, but can choose to ignore.
That's a reasonable compromise for now, thanks. Looks like it just needs a cargo fmt :)
@zesterer i think its ready now. also added BasicStyle so that users that needed custom styles would have an obvious easy way to achieve that. its defaulting to String so we dont have the lifetime issues we had in the previous version.
like you can just make those inline when you need them and it should just kinda work.
FYI, it looks like you might have accidentally piped some git log output into a file named et --soft HEAD~1 and committed it.
I think I'm less convinced by the distinct types for the report styles though (ErrorStyle, WarningStyle, etc.). A common way that Ariadne is used looks something like this:
fn emit_error(err: MyCompilerError) {
let report_kind = match err.kind {
UnexpectedToken(_) => ReportKind::Error,
TypeMismatch(_) => ReportKind::Error,
UnusedVariable(_) => ReportKind::Warning,
...
};
let mut report = ariadne::Report::build(report_kind, err.span);
match err.kind {
UnexpectedToken(tok) => report.add_label(Label::build(tok.span)),
TypeMismatch(ty_a, ty_b) => report.add_labels([
Label::build(ty_a.span),
Label::build(ty_b.span),
]),
UnusedVariable(var) => report.add_label(Label::build(var.span)),
...
}
report.eprint();
}
Note that this pattern no longer works when each ReportKind is a distinct type: you'd end up with a mismatch error in the first match.
I'm not entirely opposed to the idea in theory, but I think it needs a bit more thinking before being added to the crate's API, so I think it should be removed for the time being, if that's ok?
@zesterer the idea was that we can have both APIs but honestly i don't really see the need for the helpers API i was just adding it since #135 did but it seem to have been added out of need and not out of it being useful.
I am still not 100% sure what to do with Custom but I am thinking we make it &'static str which should remove the lifetime AND keep ReportKind copy. I cant think of a better solution.
users who need dynamic additional types can use BasicStyle or implement their own style.
I was asleep. It looks like et --soft HEAD~1 is still present, so that needs removing before merge (ideally by amending the commit). I think that's all then.