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

Added italian language support. #36

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

albertodall
Copy link

I added the italian language support.

@@ -0,0 +1,121 @@
module Ploeh.Numsense.Italian
Copy link
Owner

Choose a reason for hiding this comment

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

Doesn't this export the Ploeh.Numsense.Italian module?

Copy link
Author

Choose a reason for hiding this comment

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

You're right, I forgot the "internal" keyword inside my keyboard... ;-)

@ploeh
Copy link
Owner

ploeh commented Feb 23, 2016

Thank you for your interest in contributing to Numsense. Thus pull request looks promising 👍

@ploeh
Copy link
Owner

ploeh commented Feb 24, 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 Italian, 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 Feb 24, 2016

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

Please retweet and spread the word 😄

@IvanoBilenchi
Copy link

Couldn't spot any mistake in the unit tests. Bravo!

@giacomociti
Copy link

sorry to be pedantic but I found two little issues due to nuances of the Italian grammar:

  • an accent is needed for numbers ending in 3 (except 3 itself) e.g. 23 must be written ventitré instead of ventitre. The version without accent is often used in practice (hence may be tolerated by the parser) but the italian grammar is clear: ventitré is correct, ventitre is not;
  • the other issue is when combining 100 (cento) with 80 (ottanta).
    The correct result should be centottanta instead of centoottanta because the two adjacent o's should be collapsed into a single one.

cheers,
giacomo

@albertodall
Copy link
Author

Hi @giacomociti
I searched for some "authoritative" rules of thumb for correctly write numbers in italian, and I found only this:
http://www.treccani.it/lingua_italiana/domande_e_risposte/grammatica/grammatica_258.html
About collapsing the adjacent o's in "centottanta", Treccani says that both ways are correct, while I'm searching for the other rule about accent on ventitre.
If you have some references, let me know.
Thanks for your review,
Alberto

@giacomociti
Copy link

HI @albertodall , in fact I think the link you provided is enough to dismantle my second objection :)
I think we can certainly trust Treccani about both ways being correct. For the accent on 3 I searched for a similarly authoritative source but I couldn't find none. I hope you can find something.

Ciao

…nta" and "centoottanta" should be both parsed as 180.

Added specific tests.
@albertodall
Copy link
Author

Since centoottanta and centottanta are both correct, Numsense now parses both as 180.
Keeping "Treccani" as a reliable source, 180 is always translated as "centoottanta" (keeping the two adjacent o's).
Still searching information for the final accent on ventitre.

@IvanoBilenchi
Copy link

According to Treccani, ventitré needs to be accented (that and any other number ending with "tré", except "tre" itself, which is a monosyllable).

Source: http://www.treccani.it/enciclopedia/accento_(La_grammatica_italiana)/

… on "tre" itself).

Moved tens handling from "simplify" to the "toItalianImp" function body.
Added unit tests
@albertodall
Copy link
Author

I should have handled the accent on "tre" if found at the end of other numbers.
3 -> "tre", 23 -> "ventitré", 23000 -> "ventitremila"
The tryParse function also parses "ventitre" and "ventitre'", translating both to 23.
Now, the "Treccani" rules should have been honored. 😄

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

5 participants