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

Allow optional arguments #273

Merged
merged 3 commits into from Oct 27, 2022

Conversation

MathieuTricoire
Copy link
Contributor

@MathieuTricoire MathieuTricoire commented Oct 13, 2022

Any thoughts about implementing From for Option?:

impl<T> From<Option<T>> for FluentValue
where
    T: Into<FluentValue>

I find interesting to allow something like this:

busy = { $gender ->
    [male] Occupé
    [female] Occupée
   *[other] Non disponible
}
let gender: Option<String> = None;
let mut args = FluentArgs::new();
args.set("gender", gender.into());
let value = bundle.format_pattern(&pattern, Some(&args), &mut errors);

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.

It seems reasonable to me. I'm marking request changes for a simple unit test. Most the existing tests are more on the integration level, but a unit test here should be good enough for merging.

@MathieuTricoire
Copy link
Contributor Author

It seems reasonable to me. I'm marking request changes for a simple unit test. Most the existing tests are more on the integration level, but a unit test here should be good enough for merging.

I've added an integration test, I think it fits more with what exists, is that ok?

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.

Thanks for tests, that makes it very clear what's going on here, and thanks for the contribution!

@gregtatum gregtatum merged commit ef71027 into projectfluent:master Oct 27, 2022
@gregtatum
Copy link
Member

The CI was failing for code coverage, but it's passing in #276, so this looks fine to merge.

@MathieuTricoire MathieuTricoire deleted the optional-fluent-value branch October 27, 2022 19:37
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.

None yet

2 participants