Skip to content
This repository has been archived by the owner on Aug 13, 2020. It is now read-only.

Added Chinese #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Added Chinese #14

wants to merge 1 commit into from

Conversation

kfazi
Copy link
Contributor

@kfazi kfazi commented Jan 11, 2016

I've added traditional and simplified Chinese with normal and financial variants.
Please keep in mind I'm not a Chinese native speaker and it would be great if somebody could check my text for correctness.

Also I'm just starting with F# and any comments how can I improve my code are very welcome 😃

@ploeh
Copy link
Owner

ploeh commented Jan 12, 2016

Thank you for your interest in contributing to Numsense. FTR, I'm currently travelling, and have only limited to time to evaluate pull requests. In due time, I'll work through my backlog, but it may take weeks 😳 😓

hundredMillions = "亿"
}

let rec internal toChineseImp useAlternativeTwo characterSet x =
Copy link
Owner

Choose a reason for hiding this comment

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

Is there any case when useAlternativeTwo is false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, this is not needed here.

I added this parameter because in Chinese depending on few conditions a number two may be written in alternative form.
I think it is trivial for a user to replace the alternative form with the usual one so I assume it is better to have it always enabled.

@kfazi
Copy link
Contributor Author

kfazi commented Jan 26, 2016

I've removed unneeded useAlternativeTwo parameter.

@kfazi kfazi force-pushed the master branch 2 times, most recently from b8f151c to 7692ba3 Compare January 27, 2016 21:12

type CharacterSet =
{
negative: string;
Copy link
Owner

Choose a reason for hiding this comment

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

Since this is an internal type, it may not be that big a deal, but since CharacterSet is a type, it'd be more idiomatic to give each named element a PascalCased name: Negative, Zero, One, etc.

From an API design perspective, it doesn't matter, because the type (currently) isn't exposed to the public, but from a readability perspective, it constitutes a small 'speed bump' for the reader of the code.

@kfazi
Copy link
Contributor Author

kfazi commented Jan 29, 2016

@ploeh: Thanks for CR 😃
I've just fixed issues you reported.

@ploeh
Copy link
Owner

ploeh commented Jan 31, 2016

It builds and runs all tests without warnings on my machine, so from a technical perspective I think we're good to go 👍

As I don't read Chinese, I'd like someone who does to review that part of it, if at all possible. I don't expect any errors, but it's always good with a second pair of eyes 😄

@ploeh
Copy link
Owner

ploeh commented Jan 31, 2016

I solicited a review on Twitter: https://twitter.com/ploeh/status/693845849370169346

Please retweet and spread the word 😄

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants