- Notifications
You must be signed in to change notification settings - Fork47
Fix SourceAnnotation ranges and refactor tostd::fmt::Display#27
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
zbraniecki commentedMar 20, 2020
This PR looks awesome! Thank you so much! for |
zzau13 commentedMar 20, 2020
Ok, should panic |
zbraniecki commentedMar 20, 2020
Can you mark this PR as draft until you're ready for review? I'm excited to review it :) |
zzau13 commentedMar 21, 2020
It seems that the coverage is not correct. In thereport details: So I think there are more bugs. So I will review that part of the logic. |
std::fmt::Displayzzau13 commentedMar 22, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Should already be presentable anddebb3e2 doubles performance |
zbraniecki left a comment
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading.Please reload this page.
zzau13 commentedMar 25, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The changes are already there. A detail is that the ranges are start at 0, it should start at 1. Well but for this PR there is already enough, |
zbraniecki commentedMar 26, 2020
Thank you! This looks awesome! |
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)[CompareSource](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 themonth" (UTC), Automerge - At any time (no schedule defined).🚦 **Automerge**: Enabled.♻ **Rebasing**: Whenever PR is behind base branch, or you tick therebase/retry checkbox.🔕 **Ignore**: Close this PR and you won't be reminded about this updateagain.---- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, checkthis box---This PR was generated by [Mend Renovate](https://mend.io/renovate/).View the [repository joblog](https://developer.mend.io/github/epage/_rust).<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOS4yMDcuMSIsInVwZGF0ZWRJblZlciI6IjM5LjIwNy4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119-->Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Uh oh!
There was an error while loading.Please reload this page.
In
test_i26what 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