Skip to content

Fix SourceAnnotation ranges and refactor to std::fmt::Display - #27

Merged
zbraniecki merged 3 commits into
rust-lang:masterfrom
zzau13:master
Mar 26, 2020
Merged

zbraniecki merged 3 commits into
rust-lang:masterfrom
zzau13:master

Conversation

@zzau13

@zzau13 zzau13 commented Mar 20, 2020 •

Copy link
Copy Markdown
Contributor

In test_i26 what should be the output?
panic or anything other than the label disappearing?

I would panic or even refactor to TryFrom. What you prefer

Fix #26

@zbraniecki

Copy link
Copy Markdown
Contributor

This PR looks awesome! Thank you so much!

for test_i26 I'd panic for now. We can improve fallibility later.

@zzau13

zzau13 commented Mar 20, 2020

Copy link
Copy Markdown
Contributor Author

Ok, should panic

@zbraniecki

Copy link
Copy Markdown
Contributor

Can you mark this PR as draft until you're ready for review? I'm excited to review it :)

@zzau13

zzau13 commented Mar 21, 2020

Copy link
Copy Markdown
Contributor Author

It seems that the coverage is not correct.
Exactly tests/fixtures/no-color/multiline_annotation.yaml.

In the report details:

thread 'test_fixtures' panicked at 'SourceAnnotation range `(23, 786)` is bigger than source length `766`', src/display_list/from_snippet.rs:190:9

So I think there are more bugs. So I will review that part of the logic.

@zzau13 zzau13 changed the title [WIP] Undefined behaviour and minor issues Fix SourceAnnotation ranges and refactor to std::fmt::Display Mar 22, 2020
@zzau13

zzau13 commented Mar 22, 2020 •

Copy link
Copy Markdown
Contributor Author

Should already be presentable and debb3e2 doubles performance

@zbraniecki
zbraniecki self-requested a review March 22, 2020 18:49

@zbraniecki zbraniecki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall - very impressive! Thank you! The patch looks great and is definitely a significant clean up!

Several leftovers:

  • the format example crashes, fix the second annotation to (21,724) (I assume the new values are a fix to calculations)
  • same with the bench
  • your branch seem to have broken color mode - cargo run --example format --features color.

Comment thread src/display_list/mod.rs Outdated
@zzau13

zzau13 commented Mar 25, 2020 •

Copy link
Copy Markdown
Contributor Author

The changes are already there.

A detail is that the ranges are start at 0, it should start at 1.
We would have to put much more coverage.

Well but for this PR there is already enough,

@zzau13
zzau13 requested a review from zbraniecki March 25, 2020 18:38
@zbraniecki
zbraniecki merged commit 80ea580 into rust-lang:master Mar 26, 2020
@zbraniecki

Copy link
Copy Markdown
Contributor

Thank you! This looks awesome!

epage pushed a commit to epage/annotate-snippets-rs that referenced this pull request Sep 29, 2025
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [automod](https://redirect.github.com/dtolnay/automod) |
dev-dependencies | patch | `1.0.14` -> `1.0.15` |

---

### Release Notes

<details>
<summary>dtolnay/automod (automod)</summary>

###
[`v1.0.15`](https://redirect.github.com/dtolnay/automod/releases/tag/1.0.15)

[Compare
Source](https://redirect.github.com/dtolnay/automod/compare/1.0.14...1.0.15)

-   Documentation improvements

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "before 5am on the first day of the
month" (UTC), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend Renovate](https://mend.io/renovate/).
View the [repository job
log](https://developer.mend.io/github/epage/_rust).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOS4yMDcuMSIsInVwZGF0ZWRJblZlciI6IjM5LjIwNy4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
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.

Error from Snippet to DisplayList

2 participants