Fix eBay API initialization with site parameter#1
Conversation
yevgenko
left a comment
There was a problem hiding this comment.
thanks for fixing, overall looks good, proposed improvements to the tests
| # Change Log | ||
| All notable changes to this project will be documented in this file. | ||
|
|
||
| The format is based on [Keep a Changelog](http://keepachangelog.com/) |
There was a problem hiding this comment.
Should we merge [fix-ebay-api-initialization-with-site-param](https://github.com/main24/ebay_api/tree/fix-ebay-api-initialization-with-site-param) into master, and keep pushing PRs to master branch?
There was a problem hiding this comment.
for some reason there are many differences between master and support-catalog-product-search branch - #2. We can use our branch as a master, but it will require forth pushing to master :)
main24
left a comment
There was a problem hiding this comment.
LGTM overall, just worth removing redundant duplicate path part and omit bumping the version to avoid accumulating version conflicts.
lib/ebay_api.rb
Outdated
| require_relative "ebay_api/middlewares" | ||
| require_relative "ebay_api/exceptions" | ||
|
|
||
| I18n.load_path += Dir[File.join(GEM_ROOT, 'config', *%w[config locales ** *.{yml,yaml}])] |
There was a problem hiding this comment.
One of 'config' path parts can be removed:
| I18n.load_path += Dir[File.join(GEM_ROOT, 'config', *%w[config locales ** *.{yml,yaml}])] | |
| I18n.load_path += Dir[File.join(GEM_ROOT, 'config', *%w[locales ** *.{yml,yaml}])] |
| else | ||
| super | ||
| end | ||
| end |
There was a problem hiding this comment.
Is it really necessary to support two ways of initialization? Wdyt about keeping only one, which would be easier to maintain?
There was a problem hiding this comment.
I want to avoid affecting other parts. As you mentioned, the fork is far behind the upstream version, and I prefer not to investigate or apply fixes if we plan to rebase onto the original gem.
ebay_api.gemspec
Outdated
| Gem::Specification.new do |gem| | ||
| gem.name = "ebay_api" | ||
| gem.version = "0.0.6" | ||
| gem.version = "0.0.7" |
There was a problem hiding this comment.
As the PR is opened not against the master branch there is no need to update the version.
The fork is well behind the upstream and could conflict with the upstream versions.
46eacd6 to
d255b7d
Compare
main24
left a comment
There was a problem hiding this comment.
LGTM overall, just need to make the test env reproduce the issue. Overlooked that in the first review, sorry. Please check the comment.
| end | ||
| end | ||
|
|
||
| describe ".new" do |
There was a problem hiding this comment.
To make the test actually checking whether everything is loaded correctly it's needed to remove I8n.load_path from the spec helper. Otherwise the test will be always green regardless of the fix.
Line 22 in d255b7d
d255b7d to
9b724d1
Compare
Description
When initializing EbayAPI with the site parameter, the client fails to properly instantiate Site objects, resulting in errors:
There are 2 reasons:
I18n locale files not loading: The gem's locale files in config/locales/ are not automatically loaded when the gem is required by consuming applications, causing I18n translation lookups to fail.
Site initialization incompatibility:
Evil::Client::Dictionarypasses raw YAML data as a positional hash argument toEbayAPI::Site.new(hash), butDry::Initializerexpects keyword arguments. This results in all Site attributes being set toDry::Initializer::UNDEFINED.As a solution:
lib/ebay_api.rbto ensure translations are available when the gem is loadedEbayAPI::Siteclass to detect when a hash is passed as a positional argument and convert it to keyword arguments. This maintains compatibility with bothEvil::Client::Dictionaryinstantiation and manualEbayAPI::Sitecreation