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

fix!: Clarify Snippet API #94

Merged
merged 13 commits into from
Mar 12, 2024
Merged

fix!: Clarify Snippet API #94

merged 13 commits into from
Mar 12, 2024

Conversation

epage
Copy link
Contributor

@epage epage commented Mar 11, 2024

This is a batch API change from talking to @Muscraft about what would make the clearer, what will make it more easily adapt to rustc, and what will make it better encoded the design in the type system.

Renames:

  • Snippet -> Message
  • Slice -> Snippet
    • Slice::new to Snippet::source
  • SourceAnnotation -> Annotation
  • AnnotationType -> Level

Other breaking changes:

  • Snippet::line_start is now defaulted to 1 instead of required
  • Create Message from Level
  • Create Annotation from Level
    • Annotation::label is now optional

New:

  • Bulk-add functions to make it simpler to use builder API for automated conversion

What wasn't done:

  • Deferred: Switch Annotation from using Level to a Style { Level, Neutral, Added, Removed }
  • Deferred: Remove footer or convert it from Vec<Label> to Vec<Message>
  • Deferred: Switching fields that are Option to have constructor methods that are Option
    • A little messy for people directly making a Message
    • Simplifies use with automated conversion
    • Alts
      • Leave as-is
      • Switch to &mut builder which gets messy with the nesting builder API
  • Message::id to Message::code. I feel like the former is more semantically meaningful

epage added 3 commits March 11, 2024 15:08
I suggested this when a `Label`s title was an `Option`.
We shouldn't be creating labels with empty strings.
@epage epage force-pushed the builder branch 3 times, most recently from bc76df2 to 7893419 Compare March 12, 2024 01:03
@epage epage force-pushed the builder branch 2 times, most recently from adf2bb5 to 373ab44 Compare March 12, 2024 16:12
Copy link
Member

@Muscraft Muscraft left a comment

Choose a reason for hiding this comment

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

Thanks for this! I think this is a good change overall.

@Muscraft Muscraft merged commit 0d1364d into rust-lang:master Mar 12, 2024
12 of 13 checks passed
@epage epage deleted the builder branch March 12, 2024 19:21
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.

2 participants