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

Made Configurator.FormatterFactories public #227

Closed

Conversation

justin-edwards
Copy link
Contributor

Unless I'm mistaken this is all that's necessary to resolve #150

@justin-edwards justin-edwards changed the title Mad Configurator.FormatterFactories public Made Configurator.FormatterFactories public Apr 17, 2014
@MehdiK
Copy link
Member

MehdiK commented Apr 17, 2014

Thanks for this. That would be it although after seeing your PR, I had some more thoughts about this issue. Please check out #150 and tell us what you think.

@justin-edwards
Copy link
Contributor Author

This should handle #150 and #218 now. There are definitely things that could be cleaner(I'm not psyched with the XFactoryColections). What do you think?

@@ -1,6 +1,7 @@
###In Development
- [#217](https://github.com/Mehdik/Humanizer/pull/217): Changed OrdinalizeExtensions to better accommodate localisations. Added pt-BR and Spanish Ordinalize localisation.
- [#221](https://github.com/Mehdik/Humanizer/pull/221): Added Russian ordinalizer
- [#227](https://github.com/MehdiK/Humanizer/pull/227): Allowed for public access to Converter, Formatter, and Ordinalizer factories via Configurator. Changed default factories to use lazy loaded Converters, Formatters, and Ordinalizer rather than creating a new instance each time one is requested.
Copy link
Member

Choose a reason for hiding this comment

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

You seem to be a fair few commits behind upstream. Please fetch and rebase.

@MehdiK
Copy link
Member

MehdiK commented Apr 20, 2014

Great effort @justin-edwards. Thanks for the hard work. In general I quite like where this PR is headed; but there are a few things which I think should change which I have commented on.

I'd appreciate if you could please address these issues. Thanks a lot.


namespace Humanizer.Configuration
{
public class ConverterFactoryCollection
Copy link
Member

Choose a reason for hiding this comment

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

Why is it called "XXCollection"? It does not implement "collection" interfaces, so It is against .NET Fx naming convention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Completely agree and I guess that I forgot to include that in my comment. I'm really not happy with that naming but I couldn't come up with anything better at the moment. Do you have another suggestion?

@MehdiK
Copy link
Member

MehdiK commented Apr 25, 2014

I thought it'd be easier to talk over the code so I just created a new PR based on this (#243) with a few changes to the signature and naming of factories. Could you please review and let me know what you think?

@justin-edwards
Copy link
Contributor Author

Closing since this is wrapped into #243

@MehdiK
Copy link
Member

MehdiK commented Apr 26, 2014

Thanks for the great work and for closing this.

@MehdiK
Copy link
Member

MehdiK commented Apr 27, 2014

This is now available on NuGet as v1.25.4.

MehdiK added a commit that referenced this pull request Apr 27, 2014
@justin-edwards justin-edwards deleted the formatter-factories-edits branch April 27, 2014 15:24
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.

Make Configurator.FormatterFactories public
4 participants