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

Added Spanish ToOrdinalWords translations #188

Closed
wants to merge 2 commits into from

Conversation

thunsaker
Copy link
Contributor

Made possible by #186.

@mexx
Copy link
Collaborator

mexx commented Apr 11, 2014

Is it correct that the form of the ordinal words in Spanish changes depending on number and gender?
It's the case for Russian and German and I mind to remember it's the same in Spanish too.


namespace Humanizer.Tests.Localisation.es
{
public class NumberToOrdinalWordsTests : AmbientCulture
Copy link
Member

Choose a reason for hiding this comment

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

Now that we've merge ToOrdinalWords into NumberToWordsExtension we should move this into NumberToWordsTests. In fact I missed this on the last PR. Please move the tests to the related NumberToWordsTests classes.

Also we should rename the test methods to match the extension method names; e.g. ToWords and ToOrdinalWords. No need for Spanish as that's

@thunsaker
Copy link
Contributor Author

@mexx yes, this was a first pass. Do we have logic in place to handle gender in these situations?
@MehdiK agreed, no need to clutter up the folder with a separate file. I'll make the changes.

@thunsaker
Copy link
Contributor Author

@mexx I found #147 and #149 which address GrammaticalGender I'll take some time tonight to look at this.

@MehdiK seems I should have moved NumberToOrdinalWordsTests for English into NumberToWordsTests how do you feel about me doing that with this PR? Would that be crossing the streams?

@thunsaker
Copy link
Contributor Author

@MehdiK Also, I'll add a negative test to make sure numbers higher than 10 aren't being converted.

@MehdiK
Copy link
Member

MehdiK commented Apr 11, 2014

Thanks. Please move the English ones over too. We should've done it in the last PR; but it slipped under my radar. Please also make sure the open curlies are on a new line.

[InlineData(5, "quinto")]
[InlineData(6, "sexto")]
[InlineData(7, "séptimo")]
[InlineData(8, "optimo")]
Copy link

Choose a reason for hiding this comment

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

Hi, number 8 should be "octavo" instead of "optimo"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, fixed.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the contribution @fe-rod :)

@fe-rod
Copy link

fe-rod commented Apr 11, 2014

As @mexx said before, ordinal words also depends on gender. For example, "The first to arrive", might be "el PRIMERO en llegar" or "la PRIMERA en llegar" depending if it is a man or a woman. In this case I believe the gender may be an argument or something like that.

@thunsaker
Copy link
Contributor Author

@fe-rod I'm aware of the issue with gender, I didn't have time to do it in the first pass but wanted to provide a template for others.

I was also wondering about possible use cases when someone wants to say the 4th of the month, in Spanish you'd just say "el cuatro" not "el quarto", except the first of the month, "el primer."

@MehdiK Do we, or do we need a way to handle humanizing dates with ordinals? ie: Converting April 4, 2014 to April Fourth Two Thousand Fourteen, which can already be done in pieces, but if the Dev tries to apply ordinals to the days of the month, it doesn't work in Spanish. Slightly tangential but just something to think about with the idiosyncrasies of each language.

[InlineData(4, "quarto")]
[InlineData(5, "quinto")]
[InlineData(6, "sexto")]
[InlineData(7, "séptimo")]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fe-rod "séptimo" vs "sétimo"? This seems to be a regional difference, is it worth calling out, or just leaving it the "more correct" version "séptimo" until someone complains?

Copy link

Choose a reason for hiding this comment

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

As far as I know it's "séptimo" (native spanish speaker by the way). What you say is a rule that applies to some words depending on the region. September could be "Setiembre" or "Septiembre", but it is not the case here.

@thunsaker
Copy link
Contributor Author

Alright...I pulled in @mexx's #147 branch (locally) to get a head start on my ordinal changes with GrammaticalGender...let's just say that things did not go well.

I'd like to hold off on merging this PR until those changes have been finalized and integrated into the master branch. Once that is done the way will be paved for this and future ordinal translations.

Thanks for the work you all have been doing!

@MehdiK
Copy link
Member

MehdiK commented Apr 12, 2014

@thunsaker #147 is in now, and it's quite neat :) You might rebase on upstream and tackle this. Thanks.

WRT "ordinal" dates question, we could create an issue and tackle it later. Can you do that please?

@MehdiK
Copy link
Member

MehdiK commented Apr 12, 2014

BTW @mexx said German and Russian ordinal translation would require both grammatical gender and number. He is going to work on that later. Can you two please coordinate this together so we don't step on each others' toes?

@mexx
Copy link
Collaborator

mexx commented Apr 12, 2014

@MehdiK gender and case, number is decided by the number formatted ;)

@thunsaker
Copy link
Contributor Author

Sounds good, I'll take a look today after pulling latest to see if it meets my needs for Spanish.

@MehdiK
Copy link
Member

MehdiK commented Apr 12, 2014

Just give me a few minutes please. I am reviewing and merging #196 which resolves some of the things you were going to tackle here.

@thunsaker
Copy link
Contributor Author

@MehdiK no worries, I just woke up. 😴 Probably won't have time to look until this evening.

@MehdiK
Copy link
Member

MehdiK commented Apr 12, 2014

#196 was just merged. It adds grammatical gender for ToOrdinalWords.

@thunsaker
Copy link
Contributor Author

@MehdiK things should be good to go now with the Spanish ordinals!

@MehdiK
Copy link
Member

MehdiK commented Apr 13, 2014

Merged now. Thanks.

@MehdiK MehdiK closed this Apr 13, 2014
@thunsaker thunsaker deleted the es-ToOrdinalWords branch April 13, 2014 08:10
@MehdiK
Copy link
Member

MehdiK commented Apr 14, 2014

This is now on NuGet as v1.22.1. Thanks for the contribution.

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.

4 participants