Skip to content

Factor out pluralisation checks in diagnostics #64238

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
varkor opened this issue Sep 6, 2019 · 8 comments · Fixed by #64280 or #64342
Closed

Factor out pluralisation checks in diagnostics #64238

varkor opened this issue Sep 6, 2019 · 8 comments · Fixed by #64280 or #64342
Assignees
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@varkor
Copy link
Member

varkor commented Sep 6, 2019

The following pattern is common in diagnostic messages in rustc:

format!("found {} thing{}", x, if x != 1 { "s" } else { "" })

It would be good to extract all of these pluralisation checks into a function or macro:

format!("found {} thing{}", x, pluralise(x))

There's already one in src/librustc/ty/error.rs, but it's not used anywhere else, so we could move it to somewhere in src/librustc_errors, and replace the occurrences of this pattern with the macro.

macro_rules! pluralise {
($x:expr) => {
if $x != 1 { "s" } else { "" }
};
}

This issue has been assigned to @V1shvesh via this comment.

@varkor varkor added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Sep 6, 2019
@Centril Centril added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. E-help-wanted Call for participation: Help is requested to fix this issue. labels Sep 6, 2019
@V1shvesh
Copy link
Contributor

V1shvesh commented Sep 7, 2019

I'd love to be assigned to this issue!

@Centril
Copy link
Contributor

Centril commented Sep 7, 2019

@rustbot assign @V1shvesh

@rustbot rustbot self-assigned this Sep 7, 2019
@V1shvesh
Copy link
Contributor

V1shvesh commented Sep 7, 2019

So, I have moved the macro under src/librustc_errors in a file called pluralise.rs. I have also located all the occurrences of the pattern.

I am a bit new to Rust, so I still am learning about the module system. How shall I pull the macro into the scope of all the relevant files?

@varkor
Copy link
Member Author

varkor commented Sep 7, 2019

@V1shvesh: I'd put it in src/librustc_errors/lib.rs, so we don't have one new file for a single macro. Then you can use it from use syntax::errors;.

@V1shvesh
Copy link
Contributor

V1shvesh commented Sep 7, 2019

Thanks for pointing me in the right direction @varkor!

@varkor
Copy link
Member Author

varkor commented Sep 9, 2019

@V1shvesh: searching for { "s" } else { "" } and { "" } else { "s" } in rustc, I can see a few cases that weren't caught by #64280, e.g. in resolve_lifetime.rs, diagnostics.rs, etc. Let's keep this issue open until they've all been replaced with the new macro.

@varkor varkor reopened this Sep 9, 2019
@glorv
Copy link
Contributor

glorv commented Sep 10, 2019

I have open a pr to refactor the pluralisations remain, @Centril please have a look at it, thanks.

@V1shvesh
Copy link
Contributor

@varkor Oh, my bad. @glorv thanks for helping out!

Centril added a commit to Centril/rust that referenced this issue Sep 20, 2019
factor out pluralisation remains after rust-lang#64280

there are two case that doesn't not match the original macro pattern at [here](https://github.com/rust-lang/rust/blob/master/src/librustc_lint/unused.rs#L146) and [here](https://github.com/rust-lang/rust/blob/master/src/libsyntax/parse/diagnostics.rs#L539) as the provided param is already a bool or the check condition is not `x != 1`, so I change the macro accept a boolean expr instead of number to fit all the cases.

@Centril  please review

Fixes rust-lang#64238.
Centril added a commit to Centril/rust that referenced this issue Sep 20, 2019
factor out pluralisation remains after rust-lang#64280

there are two case that doesn't not match the original macro pattern at [here](https://github.com/rust-lang/rust/blob/master/src/librustc_lint/unused.rs#L146) and [here](https://github.com/rust-lang/rust/blob/master/src/libsyntax/parse/diagnostics.rs#L539) as the provided param is already a bool or the check condition is not `x != 1`, so I change the macro accept a boolean expr instead of number to fit all the cases.

@Centril  please review

Fixes rust-lang#64238.
tmandry added a commit to tmandry/rust that referenced this issue Sep 20, 2019
factor out pluralisation remains after rust-lang#64280

there are two case that doesn't not match the original macro pattern at [here](https://github.com/rust-lang/rust/blob/master/src/librustc_lint/unused.rs#L146) and [here](https://github.com/rust-lang/rust/blob/master/src/libsyntax/parse/diagnostics.rs#L539) as the provided param is already a bool or the check condition is not `x != 1`, so I change the macro accept a boolean expr instead of number to fit all the cases.

@Centril  please review

Fixes rust-lang#64238.
tmandry added a commit to tmandry/rust that referenced this issue Sep 20, 2019
factor out pluralisation remains after rust-lang#64280

there are two case that doesn't not match the original macro pattern at [here](https://github.com/rust-lang/rust/blob/master/src/librustc_lint/unused.rs#L146) and [here](https://github.com/rust-lang/rust/blob/master/src/libsyntax/parse/diagnostics.rs#L539) as the provided param is already a bool or the check condition is not `x != 1`, so I change the macro accept a boolean expr instead of number to fit all the cases.

@Centril  please review

Fixes rust-lang#64238.
tmandry added a commit to tmandry/rust that referenced this issue Sep 20, 2019
factor out pluralisation remains after rust-lang#64280

there are two case that doesn't not match the original macro pattern at [here](https://github.com/rust-lang/rust/blob/master/src/librustc_lint/unused.rs#L146) and [here](https://github.com/rust-lang/rust/blob/master/src/libsyntax/parse/diagnostics.rs#L539) as the provided param is already a bool or the check condition is not `x != 1`, so I change the macro accept a boolean expr instead of number to fit all the cases.

@Centril  please review

Fixes rust-lang#64238.
tmandry added a commit to tmandry/rust that referenced this issue Sep 20, 2019
factor out pluralisation remains after rust-lang#64280

there are two case that doesn't not match the original macro pattern at [here](https://github.com/rust-lang/rust/blob/master/src/librustc_lint/unused.rs#L146) and [here](https://github.com/rust-lang/rust/blob/master/src/libsyntax/parse/diagnostics.rs#L539) as the provided param is already a bool or the check condition is not `x != 1`, so I change the macro accept a boolean expr instead of number to fit all the cases.

@Centril  please review

Fixes rust-lang#64238.
tmandry added a commit to tmandry/rust that referenced this issue Sep 21, 2019
factor out pluralisation remains after rust-lang#64280

there are two case that doesn't not match the original macro pattern at [here](https://github.com/rust-lang/rust/blob/master/src/librustc_lint/unused.rs#L146) and [here](https://github.com/rust-lang/rust/blob/master/src/libsyntax/parse/diagnostics.rs#L539) as the provided param is already a bool or the check condition is not `x != 1`, so I change the macro accept a boolean expr instead of number to fit all the cases.

@Centril  please review

Fixes rust-lang#64238.
Centril added a commit to Centril/rust that referenced this issue Sep 21, 2019
factor out pluralisation remains after rust-lang#64280

there are two case that doesn't not match the original macro pattern at [here](https://github.com/rust-lang/rust/blob/master/src/librustc_lint/unused.rs#L146) and [here](https://github.com/rust-lang/rust/blob/master/src/libsyntax/parse/diagnostics.rs#L539) as the provided param is already a bool or the check condition is not `x != 1`, so I change the macro accept a boolean expr instead of number to fit all the cases.

@Centril  please review

Fixes rust-lang#64238.
@bors bors closed this as completed in d021dba Sep 21, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
5 participants