-
Notifications
You must be signed in to change notification settings - Fork 0
Fix eBay API initialization with site parameter #1
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
Fix eBay API initialization with site parameter #1
Conversation
yevgenko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
| I18n.load_path += ["config/locales/en.yml"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good spot! thanks
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