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

Improve handling of numbers in IntoDiagnosticArg #120398

Merged
merged 2 commits into from
Jan 27, 2024

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Jan 26, 2024

While working on #120393, I realize that my fluent selectors were not working. So here is an improvement (not a fix unfortunately).

@rustbot
Copy link
Collaborator

rustbot commented Jan 26, 2024

r? @TaKO8Ki

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 26, 2024
compiler/rustc_errors/src/diagnostic_impls.rs Outdated Show resolved Hide resolved
Comment on lines +71 to +75
if let Ok(n) = TryInto::<i128>::try_into(self) && n >= -100 && n <= 100 {
DiagnosticArgValue::Number(n)
} else {
self.to_string().into_diagnostic_arg()
}
Copy link
Member

Choose a reason for hiding this comment

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

Hm, this seems quite confusing. If someone is matching on different numbers, say 0, 1, 100, and 1000, then the latter one will be different, which is pretty inconsistent.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's a necessary compromise given the usage of f64 in FluentNumber. In reality, you're probably using one of the CLDR numerical selectors, rather than the number directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. I thought of using a more complicated system which would take into account f64 maximum representable base-10 number but I think it's overkill.

The reason I wanted to match on the numbers was to change the sentence in the 1 case, and it will be the case of many other peoples and I think that in many languages phrases with numbers generally only care about 0, 1, >1. So to be safe I choose 100 as a low but consistent and predictable number.

Co-authored-by: Michael Goulet <michael@errs.io>
@compiler-errors
Copy link
Member

I think this is a good step even if it's a bit inconsistent.

@bors r+

@bors
Copy link
Contributor

bors commented Jan 27, 2024

📌 Commit 93ff4a4 has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 27, 2024
Nadrieril added a commit to Nadrieril/rust that referenced this pull request Jan 27, 2024
…piler-errors

Improve handling of numbers in `IntoDiagnosticArg`

While working on rust-lang#120393, I realize that my fluent selectors were not working. So here is an improvement (not a fix unfortunately).
Nadrieril added a commit to Nadrieril/rust that referenced this pull request Jan 27, 2024
…piler-errors

Improve handling of numbers in `IntoDiagnosticArg`

While working on rust-lang#120393, I realize that my fluent selectors were not working. So here is an improvement (not a fix unfortunately).
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 27, 2024
Rollup of 9 pull requests

Successful merges:

 - rust-lang#111379 (Boost iterator intersperse(_with) performance)
 - rust-lang#118182 (Properly recover from trailing attr in body)
 - rust-lang#119641 (Remove feature not required by `Ipv6Addr::to_cononical` doctest)
 - rust-lang#119759 (Add FileCheck annotations to dataflow-const-prop tests)
 - rust-lang#120275 (Avoid ICE in trait without `dyn` lint)
 - rust-lang#120376 (Update codegen test for LLVM 18)
 - rust-lang#120386 (ScopeTree: remove destruction_scopes as unused)
 - rust-lang#120398 (Improve handling of numbers in `IntoDiagnosticArg`)
 - rust-lang#120399 (Remove myself from review rotation)

r? `@ghost`
`@rustbot` modify labels: rollup
Nadrieril added a commit to Nadrieril/rust that referenced this pull request Jan 27, 2024
…piler-errors

Improve handling of numbers in `IntoDiagnosticArg`

While working on rust-lang#120393, I realize that my fluent selectors were not working. So here is an improvement (not a fix unfortunately).
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 27, 2024
Rollup of 9 pull requests

Successful merges:

 - rust-lang#111379 (Boost iterator intersperse(_with) performance)
 - rust-lang#118182 (Properly recover from trailing attr in body)
 - rust-lang#119641 (Remove feature not required by `Ipv6Addr::to_cononical` doctest)
 - rust-lang#119957 (fix: correct suggestion arg for impl trait)
 - rust-lang#120275 (Avoid ICE in trait without `dyn` lint)
 - rust-lang#120376 (Update codegen test for LLVM 18)
 - rust-lang#120386 (ScopeTree: remove destruction_scopes as unused)
 - rust-lang#120398 (Improve handling of numbers in `IntoDiagnosticArg`)
 - rust-lang#120399 (Remove myself from review rotation)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 27, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#118182 (Properly recover from trailing attr in body)
 - rust-lang#119641 (Remove feature not required by `Ipv6Addr::to_cononical` doctest)
 - rust-lang#119957 (fix: correct suggestion arg for impl trait)
 - rust-lang#120386 (ScopeTree: remove destruction_scopes as unused)
 - rust-lang#120398 (Improve handling of numbers in `IntoDiagnosticArg`)
 - rust-lang#120399 (Remove myself from review rotation)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0972d44 into rust-lang:master Jan 27, 2024
11 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 27, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 27, 2024
Rollup merge of rust-lang#120398 - Urgau:into_diag_arg-numbers, r=compiler-errors

Improve handling of numbers in `IntoDiagnosticArg`

While working on rust-lang#120393, I realize that my fluent selectors were not working. So here is an improvement (not a fix unfortunately).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants