Skip to content

Null I18n#1

Closed
isaacseymour wants to merge 1 commit into
dgilperez:pull-259from
gocardless:null-i18n
Closed

Null I18n#1
isaacseymour wants to merge 1 commit into
dgilperez:pull-259from
gocardless:null-i18n

Conversation

@isaacseymour
Copy link
Copy Markdown

@dgilperez @avit this adds a wrapper around I18n (IceCube::I18n) which attempts to require in the real library, and delegate to that, but uses IceCube::NullI18n instead if that fails, which implements the very very basics of the real library's interpolation logic to get by. I don't hugely like it but it does get rid of the runtime dependency.

@dgilperez
Copy link
Copy Markdown
Owner

@isaacseymour thanks for the PR, awesome job! I also think it does the job pretty well to avoid the dependency. My only concern is that the IceCube::NullI18n class may grow and grow with new requirements in the future, mimicking the real one ... but, hey, that's the purpose and the price to pay to stay dependency-less. It's fine for me, if @avit agrees I'd merge it right away.

@isaacseymour
Copy link
Copy Markdown
Author

That's exactly my concern, but don't think there's any other way of avoiding the dependency :(

@dgilperez
Copy link
Copy Markdown
Owner

Hey @avit, we're also waiting for your input here. This looks like a viable solution for the concern about dependencies you mentioned here: ice-cube-ruby#273

@dgilperez
Copy link
Copy Markdown
Owner

@seejohnrun we're also waiting for some input here. Thanks for having a look!

@greysteil
Copy link
Copy Markdown

Would love to see this merged. @seejohnrun / @avit - do you have a moment to review?

@seejohnrun
Copy link
Copy Markdown

@greysteil I should be able to get to it at some point this week!

@dgilperez
Copy link
Copy Markdown
Owner

@seejohnrun 👏

@seejohnrun
Copy link
Copy Markdown

I had a 👶 !
Can we get a PR for this against seejohnrun/ice_cube I'd love to see this land

@greysteil
Copy link
Copy Markdown

@seejohnrun - congrats!

Everything's now in ice-cube-ruby#311

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