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

Add dynamic diagnostic #262

Merged
merged 30 commits into from
May 13, 2023
Merged

Add dynamic diagnostic #262

merged 30 commits into from
May 13, 2023

Conversation

gavrilikhin-d
Copy link
Contributor

Draft for dynamic diagnostics for design discussion.

Implements #259

@gavrilikhin-d
Copy link
Contributor Author

Do you agree with DynamicDiagnostic name?

@gavrilikhin-d
Copy link
Contributor Author

gavrilikhin-d commented May 5, 2023

How generic should this class be?

I tried using generic types with defaults:

struct DynamicDiagnostic<D = String, C = String, ...> 
where
    D: Debug + Display,
    C: Debug + Display,
    ...
{
   pub description: D,
   pub code: Option<C>,
   ...
}

But this was ambiguous if using None values:

let diag: DynamicDiagnostic<&str, String, ...> = DynamicDiagnostic {
   description: "",
   code: None,
   ...
};

Copy link
Owner

@zkat zkat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far! I don't think this needs to be generic, tbh

src/dynamic_diagnostic.rs Outdated Show resolved Hide resolved
src/dynamic_diagnostic.rs Outdated Show resolved Hide resolved
src/dynamic_diagnostic.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@gavrilikhin-d gavrilikhin-d changed the title Draft: Add dynamic diagnostic Add dynamic diagnostic May 6, 2023
@gavrilikhin-d gavrilikhin-d requested a review from zkat May 6, 2023 15:32
Copy link
Owner

@zkat zkat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking really great so much! Thanks for taking this on! Just a couple more comments and I think we're good to merge?

src/protocol.rs Show resolved Hide resolved
src/protocol.rs Outdated Show resolved Hide resolved
src/miette_diagnostic.rs Show resolved Hide resolved
src/miette_diagnostic.rs Outdated Show resolved Hide resolved
src/eyreish/macros.rs Outdated Show resolved Hide resolved
src/eyreish/macros.rs Outdated Show resolved Hide resolved
@gavrilikhin-d gavrilikhin-d requested a review from zkat May 7, 2023 02:31
@gavrilikhin-d
Copy link
Contributor Author

is cargo readme from #124 not longer working?

@zkat
Copy link
Owner

zkat commented May 12, 2023

is cargo readme from #124 not longer working?

@gavrilikhin-d cargo make readme

Copy link
Owner

@zkat zkat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're good now! I know this was a lot of back and forth but I think we got somewhere very nice. Thanks so much for your time and effort!!

tests/test_chain.rs Show resolved Hide resolved
@zkat
Copy link
Owner

zkat commented May 12, 2023

looks like you have a clippy thing too

@gavrilikhin-d gavrilikhin-d requested a review from zkat May 13, 2023 01:38
@gavrilikhin-d
Copy link
Contributor Author

Issues #259 and #201 will be resolved by this pr

@gavrilikhin-d
Copy link
Contributor Author

Why tests fail on windows? Looks like it can't find local arguments and assumes they are named arguments

@zkat
Copy link
Owner

zkat commented May 13, 2023

I think 1.56.0 is before format argument interpolation was stabilized, now that I think about it

@gavrilikhin-d
Copy link
Contributor Author

@zkat how can I fix it? See no way to conditionally compile code depending on rust version.

My best guess is to introduce new default feature format_args_capture

@gavrilikhin-d
Copy link
Contributor Author

Now it will pass workflow (checked in my fork)

@zkat zkat merged commit 024145d into zkat:main May 13, 2023
@zkat
Copy link
Owner

zkat commented May 13, 2023

Yay! It's in!!

@gavrilikhin-d gavrilikhin-d deleted the dynamic-diagnostic branch May 14, 2023 12:02
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 this pull request may close these issues.

Add struct for dynamic creation of diagnostics miette! does not support capturing names in the current scope
3 participants