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

Let Vec be used as an argument in fluent strings #106986

Closed
wants to merge 2 commits into from
Closed

Let Vec be used as an argument in fluent strings #106986

wants to merge 2 commits into from

Conversation

mejrs
Copy link
Contributor

@mejrs mejrs commented Jan 17, 2023

An alternative approach would be to have some kind of #[list] attribute for the derive macro to use, or a newtype for doing this. I chose the current approach because it's what people will try first and it'd be nice to have it "Just Work" - there's no need to write or read documentation.

@rustbot
Copy link
Collaborator

rustbot commented Jan 17, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @davidtwco (or someone else) soon.

Please see the contribution instructions for more information.

@rustbot rustbot added A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic 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 17, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jan 17, 2023

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

@Noratrieb
Copy link
Member

I think @Manishearth wanted to use ICU4X for this IIRC

@mejrs
Copy link
Contributor Author

mejrs commented Jan 18, 2023

I think @Manishearth wanted to use ICU4X for this IIRC

This uses the list support added in #104047 . Other than the "x more" part, which I couldn't figure out - is that even supported?

Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

This shouldn't be done this way: to handle "or more" you should be having the logic in the fluent file, something like

mir_build_uncovered = {$truncated ->
        [true] {$witnesses}
        *[other] {$witnesses}, and more
    } not covered

and the truncation logic being handled in Rust. Except that's not so straightforward since the list will already format with a , and. You might need to define a fluent custom function that is able to do list formatting, cc @davidtwco. Unfortunatley it doesn't have a list type yet.

Furthermore, not all lists will want to be truncated. We should have a TruncatedDiagnosticList type that has all this logic.

This would be easier if Fluent had builtin list formatting support (projectfluent/fluent#79)


I think the way to do this for now is have another internal list type (perhaps we can merge it all into StrList, which has And/Or and truncate/don't truncate options), and have it pull from a list_more = and {$count} more Fluent key that's "global"

@mejrs
Copy link
Contributor Author

mejrs commented Jan 22, 2023

Thanks, I've just created a new type for it.

and have it pull from a list_more = and {$count} more Fluent key that's "global"

Where should this live?

@davidtwco
Copy link
Member

Where should this live?

We could create a common.ftl for it?

@mejrs
Copy link
Contributor Author

mejrs commented Feb 2, 2023

I'm not sure how to make it fit together though. I assume I'd need to get access to the bundle inside the IntoDiagnosticArg impl, but that doesn't seem quite right. Is my approach wrong?

@davidtwco
Copy link
Member

I'm not sure how to make it fit together though. I assume I'd need to get access to the bundle inside the IntoDiagnosticArg impl, but that doesn't seem quite right. Is my approach wrong?

Is it possible with the way we use IntoDiagnosticArg that you could give it a reference to the Handler and use eager_translate?

@mejrs mejrs closed this Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants