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

Warn when redefining a filter #310

Merged
merged 6 commits into from Oct 16, 2015
Merged

Warn when redefining a filter #310

merged 6 commits into from Oct 16, 2015

Conversation

tfausak
Copy link
Collaborator

@tfausak tfausak commented Oct 15, 2015

This pull request makes it so that you get a warning when you redefine a filter. Issue #308 brought the need for this to my attention.

Here is an example of the warning:

class Example < ActiveInteraction::Base
  boolean :x, :x
end
# WARNING: Redefining Example#x filter!

@tfausak tfausak self-assigned this Oct 15, 2015
@tfausak
Copy link
Collaborator Author

tfausak commented Oct 15, 2015

I am open to better wording. As is, it gives you enough information to figure out what's going on. But we may want to include more, like filter types.

@tfausak
Copy link
Collaborator Author

tfausak commented Oct 15, 2015

The build failed because RuboCop thinks ActiveInteraction::Base has too many lines. We can either increase the maximum class length or figure out how to break it up.

@AaronLasseigne
Copy link
Owner

Is there any case where you might intentionally redefine a filter? I can't really think of one. Maybe on some kind of conditional filter import? Should we just raise an error instead?

Also, I don't think it needs to be so excited (i.e. we can drop the "!").

@tfausak
Copy link
Collaborator Author

tfausak commented Oct 15, 2015

I can get behind raising an error instead of a warning. But that would be a major change to our API, right? And if we did raise an error, would we want to allow users to ignore it?

There has never been an instance where I wanted to redefine a filter. I do sometimes want to modify them, but I use .filters for that.

@AaronLasseigne
Copy link
Owner

Maybe it's best that we just warn. There might be some situation we're not considering where it makes sense.

BTW, the code looks 👍

tfausak added a commit that referenced this pull request Oct 16, 2015
@tfausak tfausak merged commit 252fbc5 into master Oct 16, 2015
@tfausak tfausak deleted the gh-308-warn-redefine branch October 16, 2015 13:52
@tfausak tfausak mentioned this pull request Nov 9, 2015
@MustafaZain
Copy link

@tfausak What if I have something like this

class Base < ActiveInteraction::Base
    object :user
    object :address
end

and another class that will inherit from Base and want the address for it to be optional for the other class

class ChildClass < Base
    object :address, default: nil
end

I got a warning WARNING: Redefining while this is intended 🤔

@tfausak
Copy link
Collaborator Author

tfausak commented Mar 3, 2022

I think you can use silence_warnings, as suggested here: #324 (comment). This might work:

class ChildClass < Base
  silence_warnings do
    object :address, default: nil
  end
end

But I haven't written Ruby in a while, so I'm not sure.

@MustafaZain
Copy link

Thanks @tfausak, It worked

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants