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 ToOrdinalWords extension with gender overload #500

Closed
wants to merge 3 commits into from

Conversation

RobPethick
Copy link

Fixes #328

{
var date = new DateTime(2015, 1, 1);

Assert.Equal("1º janeiro 2015", date.ToOrdinalWords(GrammaticalGender.Masculine));
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it be automatically?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@hazzik what do you target with this question, the gender parameter?
I also thought it should be tied to a default value, the Ordinalize extensions also do it that way. By this the localization can decide which gender to pick as default.

Copy link
Member

Choose a reason for hiding this comment

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

It's about the question on the main thread. For example, in russian the gender of the number in a date is always neutral (средний род), and for some languages it can be different. I don't think that we need this method overload at all.

But, for russian we need an overload with grammatical case, to say "1-го апреля 2016" (at 1st April 2015), for example.

@hazzik
Copy link
Member

hazzik commented Dec 17, 2015

In portugise, on what the gender of a number is based? Month? Year? Number itself? Or some other word?

/// <returns>The date in ordinal words</returns>
public static string ToOrdinalWords(this DateTime input)
{
return input.Day.Ordinalize() + input.ToString(" MMMM yyyy");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder whether it works for all locales?
How do Americans write such dates? Wasn't it January 1st 2015?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, sure, this is language specific.

Copy link
Author

Choose a reason for hiding this comment

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

Good point, there are plenty of countries that do MMDDYY and plenty that do YYMMDD. I'm working on an update to be localised. Can I ask if I just put up a PR request with only a couple of languages is that ok? Others could then add more in other languages?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, just provide as minimum the implementation for English, other languages can then localize this functionality with own PRs. See for example #493

@mexx
Copy link
Collaborator

mexx commented Dec 17, 2015

Please also extend the readme.md with the documentation of this extension.

@RobPethick
Copy link
Author

Should the readme.md use en-GB or en-US as default examples? Or just both?

@mexx
Copy link
Collaborator

mexx commented Dec 21, 2015

@RobPethick please lean on number to ordinal words section.

@RobPethick
Copy link
Author

@mexx thanks but I think in that case en-GB and en-US are the same? And in my case they are different ( 1st January 2015 vs January 1st, 2015)

@mexx
Copy link
Collaborator

mexx commented Dec 21, 2015

I don't get your point, the documentation should show that this functionality is available. An example in either locale would be sufficient.

@RobPethick
Copy link
Author

@mexx Thanks! I was just checking there wasn't an existing convention to adhere to.

@clairernovotny
Copy link
Member

@RobPethick Just a head's up that it'd be great to resolve this in the next couple of weeks if you'd like to get this into 2.0. I'll leave it to @mexx as to when it's ready.

@@ -263,4 +264,4 @@
<Target Name="AfterBuild">
</Target>
-->
</Project>
</Project>
Copy link
Collaborator

Choose a reason for hiding this comment

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

changes to this file should be reverted, as they only formatting and add no value to the feature.

@mexx mexx added this to the vNext milestone Jan 2, 2016
@mexx
Copy link
Collaborator

mexx commented Jan 2, 2016

Tagging as vNext as the same thing as @hazzik mentioned for ToQuantity applies here:

I don't think that we can fix this in a couple of weeks. To make it feature complete we need to complete a lot of languages.

@RobPethick
Copy link
Author

@onovotny @mexx cool I am back at work now after Christmas so should get this finished up soon!

@clairernovotny
Copy link
Member

@RobPethick @mexx is this ready to go once the conflicts are resolved? As to support for the other languages, I don't think we need to hold this up -- we can always release again once more languages are added.

@clairernovotny
Copy link
Member

rebased and merged here 3d71fd9, thanks!

@clairernovotny clairernovotny modified the milestones: v2.2, vNext May 4, 2017
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