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

Add options for cardinal/ordinal plural types #259

Closed
wants to merge 1 commit into from

Conversation

oliver-ni
Copy link
Contributor

Adds functionality for switching between plural types (cardinal and ordinal).

Fluent docs include this option in the NUMBER built-in function. Since fluent-rs doesn't have that yet, I included it in the place where all the other number options were. Please let me know if this isn't the appropriate place / way to include this functionality.

Also, I called it kind instead of type since the latter is a Rust keyword... a little weird but not sure if there's a better way to deal with that.

@juliancoffee
Copy link
Contributor

@oliver-ni to answer on using "type" in Rust, you can use raw identifiers.
https://doc.rust-lang.org/rust-by-example/compatibility/raw_identifiers.html

Copy link
Member

@gregtatum gregtatum left a comment

Choose a reason for hiding this comment

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

The changes look reasonable, but I have a few requested changes.

@oliver-ni to answer on using "type" in Rust, you can use raw identifiers.

Thought: I think I prefer kind here, it's used other places in the codebase.

Suggestion: This code is missing tests. It would probably be good to add some unit tests to the file, and see if there are relevant areas that an integration test could be added.

@@ -8,6 +8,28 @@ use intl_pluralrules::operands::PluralOperands;
use crate::args::FluentArgs;
use crate::types::FluentValue;

#[derive(Debug, Copy, Clone, Hash, PartialEq, Eq)]
pub enum FluentNumberKind {
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: Please add docs here, with a link to the relevant Fluent documentation, and an example of what the .ftl syntax looks like.

@gregtatum
Copy link
Member

gregtatum commented Sep 8, 2023

Closing as stale. Feel free to re-open if you want to address the requested changes.

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.

3 participants