Skip to content

Conversation

@ulandj
Copy link
Collaborator

@ulandj ulandj commented Nov 20, 2025

Description

When initializing EbayAPI with the site parameter, the client fails to properly instantiate Site objects, resulting in errors:

EbayAPI.new(token: 'some', site: 'EBAY-US', sandbox: false)
# => #<EbayAPI:0x000000001188e8 @token=some, @site=Translation missing: en.evil.client.sites.EBAY-US, @charset=utf-8, @sandbox=false, @gzip=false>

There are 2 reasons:

  1. 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.

  2. Site initialization incompatibility: Evil::Client::Dictionary passes raw YAML data as a positional hash argument to EbayAPI::Site.new(hash), but Dry::Initializer expects keyword arguments. This results in all Site attributes being set to Dry::Initializer::UNDEFINED.

As a solution:

  1. Added I18n locale path registration in lib/ebay_api.rb to ensure translations are available when the gem is loaded
  2. Modified EbayAPI::Site class to detect when a hash is passed as a positional argument and convert it to keyword arguments. This maintains compatibility with both Evil::Client::Dictionary instantiation and manual EbayAPI::Site creation

@ulandj ulandj requested a review from main24 November 20, 2025 09:44
Copy link

@yevgenko yevgenko left a 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/)

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?

Copy link
Collaborator Author

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 :)

Copy link
Owner

@main24 main24 left a 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}])]
Copy link
Owner

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:

Suggested change
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
Copy link
Owner

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?

Copy link
Collaborator Author

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"
Copy link
Owner

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.

@ulandj ulandj force-pushed the fix-ebay-api-initialization-with-site-param branch from 46eacd6 to d255b7d Compare November 20, 2025 17:37
Copy link
Owner

@main24 main24 left a 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
Copy link
Owner

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.

I18n.load_path += ["config/locales/en.yml"]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good spot! thanks

@ulandj ulandj force-pushed the fix-ebay-api-initialization-with-site-param branch from d255b7d to 9b724d1 Compare November 21, 2025 09:42
@ulandj ulandj merged commit 104c23b into support-catalog-product-search Nov 21, 2025
@ulandj ulandj deleted the fix-ebay-api-initialization-with-site-param branch November 21, 2025 09:43
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.

5 participants