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

Decompose I18n::Backend::Base#localize #347

Merged
merged 1 commit into from
Nov 13, 2016
Merged

Conversation

corlinus
Copy link
Contributor

@corlinus corlinus commented Nov 7, 2016

It allows to customise directives without overwriting whole localize method

@radar
Copy link
Collaborator

radar commented Nov 7, 2016

Thanks for the contribution!

Could you please demonstrate a use case of this feature?

@dmitry
Copy link

dmitry commented Nov 7, 2016

I believe it's a matter of extension format types.

@corlinus
Copy link
Contributor Author

corlinus commented Nov 7, 2016

something like

module TranslatedZoneName
  private
  def translate_format(locale, format, object)
    super.gsub(
      '%O',
      (object.respond_to?(:time_zone) ? I18n.t(:"timezones.#{object.time_zone.name}",
                                              locale: locale, default: object.time_zone.name) : '')
    )
  end
end
I18n::Backend::Base.prepend TranslatedZoneName

(does not tested. just as an expample)

@radar
Copy link
Collaborator

radar commented Nov 7, 2016

Ok, thanks. Seems like a good addition to me.

Can you please write a test for this?

@radar
Copy link
Collaborator

radar commented Nov 7, 2016

Looks like this would be a good workaround for #210.

@corlinus
Copy link
Contributor Author

corlinus commented Nov 9, 2016

I believe that there is no need to test something. I didn't change behaviour, split method though. And looks like it is not good practice to test private methods, it's better to test interface and i suppose it's already done. Maybe I don't understand somethig. Could you please explain than?

end
end

format = translate_format(locale, format, object)
Copy link

Choose a reason for hiding this comment

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

Might be good to have the same method signature as parent has:

locale, object, format = :default, options = {}

Copy link
Contributor Author

@corlinus corlinus Nov 9, 2016

Choose a reason for hiding this comment

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

Do you mean that it would be better to have access to options inside translate_format for future customisation? format doen't need default value it will always set from parent method

@corlinus
Copy link
Contributor Author

Hi @radar, so how do you think? could we merge or you still think I should write some tests on it?

@radar
Copy link
Collaborator

radar commented Nov 13, 2016

@corlinus I think it's fine to merge without them. I can't work out a way to write tests myself.

@radar radar merged commit ae84ad3 into ruby-i18n:master Nov 13, 2016
@radar
Copy link
Collaborator

radar commented Nov 13, 2016

Thanks for your contribution!

@radar radar added this to the 0.8.0 milestone Nov 13, 2016
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.

None yet

3 participants