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

Merge measured-rails into this gem #155

Merged
merged 2 commits into from
Apr 16, 2024
Merged

Merge measured-rails into this gem #155

merged 2 commits into from
Apr 16, 2024

Conversation

paracycle
Copy link
Member

@paracycle paracycle commented Apr 15, 2024

This is an initial attempt at reducing the support burden of having 2 separate gems for measured behaviour, when we can easily have a single gem that (conditionally) loads a Railtie which does all Active Record registrations.

This allows the gem to work as a simple Ruby gem, as well as a Rails extension.

This PR copies all of the current behaviour of measured-rails into this gem and makes it load when ActiveRecord is loaded.

Copy link
Member

@kmcphillips kmcphillips left a comment

Choose a reason for hiding this comment

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

This is good!

The README needs to be updated to include all the stuff in the measured-rails about how to use the Rails stuff. It can mostly just be copied over and put into another top level heading.

It'll mean a major version bump of this gem, and then measured-rails should have a post install message that says it does nothing and then make the gem an empty shell, and a README that just points to this repo, and then release them both to a major version at the same time. Then just stop supporting the old one.

I think that will work? If someone just says gem "measured-rails" in their project it will update to the new major version then it will not work at all, and they can see the post install message and just change it to gem "measured". Shouldn't be dangerous, should it?

Thanks for doing this. It simplifies it quite a bit.


::ActiveRecord::Base.include(
Measured::Rails::ActiveRecord,
Measured::Rails::ActiveRecord::Length,
Copy link
Member

Choose a reason for hiding this comment

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

This was like this before, but I think we can lazy load these. I'll make a note to look at this later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I am doing a more or less straight port for now, and trying to not do any incidental changes.

It would definitely be great if we can do the lazy loading.

Copy link
Member

Choose a reason for hiding this comment

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

It's how measured works, or at least optionally. But seeing this in one place makes me realize that we don't allow that if you're using measured-rails. I'll put it on my backlog.

class Rails < ::Rails::Railtie
class Error < StandardError ; end

ActiveSupport.on_load(:active_record) do
Copy link
Member

Choose a reason for hiding this comment

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

This plugin is also designed to work on an ActiveModel::Model. Though apparently it isn't tested.

@paracycle
Copy link
Member Author

I merged the README sections from the measured-rails gem into this gem's README too now.

@paracycle paracycle force-pushed the uk-merge-measured-rails branch from 53e5807 to 448bd1b Compare April 15, 2024 21:59
@@ -269,7 +379,7 @@ Existing alternatives which were considered:
* **Cons**
* Opens up and modifies `Array`, `Date`, `Fixnum`, `Math`, `Numeric`, `String`, `Time`, and `Object`, then depends on those changes internally.
* Lots of code to solve a relatively simple problem.
* No ActiveRecord adapter.
* No Active Record adapter.
Copy link
Member

Choose a reason for hiding this comment

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

😬

@paracycle paracycle changed the title Merge measured-rails into this gem Merge `measured-rails into this gem Apr 16, 2024
@paracycle paracycle changed the title Merge `measured-rails into this gem Merge measured-rails into this gem Apr 16, 2024
@paracycle paracycle merged commit 9bc54ce into main Apr 16, 2024
31 checks passed
@paracycle paracycle deleted the uk-merge-measured-rails branch April 16, 2024 19:23
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