Remove Report/ReportKind lifetime #135
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…, avoiding the extra lifetime to support custom report kinds.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR suggests a refactor of
enum ReportKind<'a>intotrait ReportKindin order to get rid of the lifetime parameter. The default kindsError,WarningandAdviceare instead represented using empty structs that implement the trait. Custom report kinds can now instead be defined by implementing theReportKindtrait for new types (with or without lifetime parameters).The motivation for this change is that the lifetime on
Report, which stems specifically fromReportKind::Custom, only serves a purpose in the specific case where a custom report kind is used with a borrowed string, while being superfluous in other cases. If you're not using custom report kinds and want to be able to return reports from functions or store them in structs, you have to stick withReport<'static>everywhere, which feels unnecessary.Bonus: a custom report kind should now only be colored if the
Confighas colors enabled. I assume this was a bug.Note that this PR contains breaking changes to the public API, specifically the change of
ReportKindfrom enum to trait, and the introduction of default trait implementations replacing the enum variants. If this is a concern, it might be possible to keep the old enum and make function signatures backwards compatible using generics, but I haven't looked into it.