Internationalization: fixing tests in pull #259 from @tomoyuki28jp#273
Internationalization: fixing tests in pull #259 from @tomoyuki28jp#273dgilperez wants to merge 8 commits into
Conversation
|
Suspicion confirmed, tests are green now. @avit @tomoyuki28jp @stravabr @joelmeyerhamme Are we ready to merge at last? |
|
Hi @avit ! Did you have time to have a look at this? I've been using my fork for a while in several pet projects and it seems to be working fine, but I'd like to see what others think. Regards! |
|
Any chance this PR gets merged anytime soon? Thanks! |
|
Yes please! 👍 |
|
Just checking in to keep this thing alive. @avit, it would be nice if you could have a look at it, or bring in some of the other maintainers. Thanks! |
|
Awesome work @dgilperez ! Would love to see this merged |
There was a problem hiding this comment.
This eliminates the option of overriding the format from the i18n config
@to_s_time_format ||= I18n.t("ice_cube.date.formats.default")maybe?
(the comment above it should be updated too)
There was a problem hiding this comment.
You could always change I18n.t("ice_cube.date.formats.default") to override that 😄, that is actually my proposed way to do that. What do you think? But maybe there are some docs / other methods to be changed accordingly?
There was a problem hiding this comment.
Yeah, actually caching is a really bad idea if you're using multiple locales in the same process (e.g. tests) - the second locale's date format gets ignored. My bad.
|
I will make the time to review this soon. This does look like a helpful addition. Currently IceCube has no runtime dependencies outside of stdlib. (This change would introduce the I18n gem, which I'm OK with since it doesn't have a whole tree of other dependencies.) Before I look at your implementation, will it be possible to make translations optional? (e.g. require "ice_cube/i18n") (Sorry this hasn't been a priority, I don't use the string output for schedule descriptions myself; I only use it for generating occurrence times.) |
|
And thanks for your work and patience, everyone. |
|
No pro @avit! Thanks for having a look at this. I will have no spare time in the following few days, so I suggest you adapt the code at this PR to the current codebase, add that option to enable / disable translations and I'll get back as soon as possible! PS: BTW, I don't think enabling / disabling translations is a good idea, or at least I don't see the use case. I'd rather add the defaults to the english |
|
@avit @dgilperez Looks great! Would be nice to get this merged soon :) |
|
👍 |
|
👍 was looking for a way to get this from |
|
So, @avit, any news? |
|
Hey @seejohnrun, are there any plans to merge this PR? Anything left to do here? |
|
Closing due to #311 been merged. |
6 participants
Hi!
I'm creating a new PR, since I don't know how to add a commit to #259. I suspect the failing tests I mentioned earlier are failing only in my machine and I'd like to see what does Travis think about that. Please feel free to delete this PR if this is not the proper way of doing it.
Regards!