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

Remove activerecord from runtime dependencies #64

Merged

Conversation

flash-gordon
Copy link
Contributor

Hey @mcmire!

It took a while but I came back.
This PR removes activerecord from runtime dependencies and runs tests in an AR-free environment to make sure activesupport doesn't interfere with the gem (I removed one .blank? usage along the way).
I didn't move code, just tagged specs and added a filter. I think you know better if tests require re-organization.
Side note: ruby 2.7 changed the meaning of .object_id, it's not related to memory address anymore. Since I locally tested on 2.7, I added a hacky helper to get an object address on the newest ruby version. Read https://bugs.ruby-lang.org/issues/15408 and https://bugs.ruby-lang.org/issues/15626#Object-ID for details. There's still one more usage of object_id in the sources, you probably want to update it too. My nasty approach works for testing, but for gem's code it can be less desirable.
I didn't add 2.7 to the matrix, I think it should be done separately. Did you think about migrating to GH actions btw?

Copy link
Collaborator

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Some minor comments from me here. Nice work!

spec/integration/rails/active_record_spec.rb Outdated Show resolved Hide resolved
spec/spec_helper.rb Outdated Show resolved Hide resolved
spec/support/object_id.rb Outdated Show resolved Hide resolved
spec/unit/object_inspection_spec.rb Outdated Show resolved Hide resolved
spec/support/object_id.rb Outdated Show resolved Hide resolved
@mcmire
Copy link
Collaborator

mcmire commented Jan 15, 2020

Oh and in response to your last comments above:

There's still one more usage of object_id in the sources, you probably want to update it too.

Okay, that's fine. I'll look more closely at what you did to see if there's another way.

I didn't add 2.7 to the matrix, I think it should be done separately.

That's fine with me.

Did you think about migrating to GH actions btw?

Yes, I tried to get Actions working when I first set up CI, but was unsuccessful. There's something about the cache action that didn't seem to be working for me, I don't know if it's a bug or what. I also don't know (or remember at the moment) how concurrency works with Actions. I've used Travis for previous projects and the matrix there makes sense to me. That said, I wouldn't mind getting Actions working eventually. There'd be one fewer place to look.

@flash-gordon
Copy link
Contributor Author

@mcmire GH actions now have matrix too, it's basically the same as it was on Travis. Here's a sample working config https://github.com/dry-rb/dry-types/blob/master/.github/workflows/ci.yml

This removes activerecord from runtime dependencies and runs tests in an AR-free environment to make sure activesupport doesn't interfere with the gem (I removed one `.blank?` usage along the way).
Side note: ruby 2.7 changed the meaning of `.object_id`, it's not related to memory address anymore. Since I locally tested on 2.7, I added a hacky helper to get an object address on the newest ruby version.
@flash-gordon
Copy link
Contributor Author

@mcmire I addressed your comments

@mcmire
Copy link
Collaborator

mcmire commented Jan 15, 2020

@flash-gordon Thanks so much!

@mcmire mcmire merged commit 6e98bd2 into splitwise:master Jan 15, 2020
@mcmire
Copy link
Collaborator

mcmire commented Jan 16, 2020

v0.4.0 is now out with this change!

@flash-gordon
Copy link
Contributor Author

Awesome! 🎉

@Mange
Copy link
Contributor

Mange commented Jan 28, 2020

This seems to have broken our build.

[2020-01-22T05:21:33.596Z] Failure/Error: require "super_diff/rspec-rails"

[2020-01-22T05:21:33.596Z] 

[2020-01-22T05:21:33.596Z] NameError:

[2020-01-22T05:21:33.596Z]   uninitialized constant ActiveRecord

[2020-01-22T05:21:33.596Z]   Did you mean?  ActiveModel

[2020-01-22T05:21:33.596Z] # /usr/local/bundle/ruby/2.6.0/gems/super_diff-0.4.0/lib/super_diff/active_record/monkey_patches.rb:2:in `<top (required)>'

[2020-01-22T05:21:33.596Z] # /usr/local/bundle/ruby/2.6.0/gems/super_diff-0.4.0/lib/super_diff/active_record.rb:39:in `require'

[2020-01-22T05:21:33.596Z] # /usr/local/bundle/ruby/2.6.0/gems/super_diff-0.4.0/lib/super_diff/active_record.rb:39:in `<top (required)>'

[2020-01-22T05:21:33.596Z] # /usr/local/bundle/ruby/2.6.0/gems/super_diff-0.4.0/lib/super_diff/rails.rb:1:in `require'

[2020-01-22T05:21:33.596Z] # /usr/local/bundle/ruby/2.6.0/gems/super_diff-0.4.0/lib/super_diff/rails.rb:1:in `<top (required)>'

[2020-01-22T05:21:33.596Z] # /usr/local/bundle/ruby/2.6.0/gems/super_diff-0.4.0/lib/super_diff/rspec-rails.rb:2:in `require'

[2020-01-22T05:21:33.596Z] # /usr/local/bundle/ruby/2.6.0/gems/super_diff-0.4.0/lib/super_diff/rspec-rails.rb:2:in `<top (required)>'

[2020-01-22T05:21:33.596Z] # ./spec/spec_helper.rb:11:in `require'

[2020-01-22T05:21:33.596Z] # ./spec/spec_helper.rb:11:in `<top (required)>'

Do we need to change some of the setup code?

@flash-gordon
Copy link
Contributor Author

@Mange to me it's unlikely changes in this PR are related. However, I think super_diff should require activerecord files itself, could you report this issue to the main repo?

@flash-gordon
Copy link
Contributor Author

#65 this is related

@Mange
Copy link
Contributor

Mange commented Jan 28, 2020

Thanks. I opened #68 to track this issue.

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.

3 participants