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

Automatic scope definition can break applications because of naming issues #7

Open
klyonrad opened this issue Sep 17, 2020 · 3 comments · May be fixed by #28
Open

Automatic scope definition can break applications because of naming issues #7

klyonrad opened this issue Sep 17, 2020 · 3 comments · May be fixed by #28

Comments

@klyonrad
Copy link

We get an error upon upgrading to boolean_timestamps 1.1.0

Sorry @jordanbyron but I think you introduced a bug with your latest feature addition 😿

Reproduction

model code

class SomeModel < ApplicationRecord
  boolean_timestamps :loaded_at
end

error

leads to

ArgumentError: You tried to define a scope named "loaded" on the model "SomeModel", but ActiveRecord::Relation already defined an instance method with the same name.

So boolean_timestamps tries to overwrite this method: https://www.rubydoc.info/docs/rails/ActiveRecord%2FRelation:loaded


Rails has some safeguards during scope definitions, See here

I propose to either respect those safeguards or make the scope definitions Opt-In/Opt-Out

@jordanbyron
Copy link

jordanbyron commented Sep 17, 2020

Ah yes that is clearly a problem. Thank you so much for letting me know. I like the idea of making these scope definitions opt-out for instances where they clash with built in methods. Is that a feature you'd be comfortable working on?

In the meantime it should be safe to downgrade to 1.0.x until this is resolved.

@klyonrad
Copy link
Author

Ah yes that is clearly a problem. Thank you so much for letting me know. I like the idea of making these scope definitions opt-out for instances where they clash with built in methods. Is that a feature you'd be comfortable working on?

No, sorry, I could not spare the working time resources on this. Personally

I think it would be faster to add an option parameter kind of like flag shih tzu does it. With such an option available gem users would at least be able to help themselves.

@jordanbyron
Copy link

jordanbyron commented Sep 17, 2020

I think it would be faster to add an option parameter kind of like flag shih tzu does it. With such an option available gem users would at least be able to help themselves.

That's exactly what I had in mind! I totally understand that you don't have time to implement this feature. Thanks for the report! 😄

tylercallahan added a commit to tylercallahan/boolean_timestamps that referenced this issue Dec 14, 2022
This adds a `:scopes` option to `boolean_timestamps` that allows the
user to skip generating the scope. This may be desirable in particular
when the generated scope conflicts with an existing Rails method.

For example, `boolean_timestamps :loaded_at` attempts to create a scope
`:loaded` and raises an error because the method `:loaded` is already
defined. Adding the `scopes: false` option avoids the error.

Fixes betterup#7
@tylercallahan tylercallahan linked a pull request Dec 14, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants