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

Require only part of activemerchant #2311

Merged

Conversation

mamhoff
Copy link
Contributor

@mamhoff mamhoff commented Oct 21, 2017

ActiveMerchant is 2.5 megs of glue code that we don't need. This PR only requires those files of ActiveMerchant that we do need: Errors, Billing Response and Credit Card.

It also bumps the required ActiveMerchant Version to 1.66, as that is the first version that works with Rails 5.1.

Copy link
Contributor

@cbrunsdon cbrunsdon left a comment

Choose a reason for hiding this comment

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

I think its probably worth pulling out that factory girl commit into another PR, or at least getting a good understanding of why we need it now...

@tvdeyen
Copy link
Member

tvdeyen commented Oct 23, 2017

factory_girl was finally renamed to factory_bot. Pulling it out into a separate PR makes sense

@tvdeyen
Copy link
Member

tvdeyen commented Oct 25, 2017

@mamhoff please rebase now that the factory bot rename is merged

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Much cleaner, less code loaded. 👍

@@ -12,6 +12,7 @@
require 'rspec/rails'
require 'ffaker'
require 'spree_sample'
require 'factory_girl'
Copy link
Contributor

Choose a reason for hiding this comment

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

This will generate a warning and I don't think it is necessary after a rebase (the Gemfile will cause factory_bot to be required).

It turns out we also don't need factories at all for this test suite, so I have removed them in #2330

ActiveMerchant comes with a large list of glue code
for gateways and more things that is not needed for
many Solidus stores.
This replaces requiring all of ActiveMerchant with just
those files/classes we need.
Versions before that do not work with Rails 5, upon which we depend.

With recent work on explicitly requiring only those bits of ActiveMerchant
that we need, I needed to test the full range of ActiveMerchant versions
for compatibility in their gem structure.

That task can only be started at the first version our gem actually bundles with.
@jhawthorn jhawthorn force-pushed the require-only-part-of-activemerchant branch from b0b7305 to b3fb4c2 Compare October 31, 2017 03:53
@jhawthorn
Copy link
Contributor

rebased and removed (now unnecessary) last commit

@jhawthorn jhawthorn merged commit 6a296d5 into solidusio:master Oct 31, 2017
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