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

Add Action Filters Order section #353

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ydakuka
Copy link

@ydakuka ydakuka commented Dec 11, 2023

@andyw8
Copy link
Contributor

andyw8 commented Dec 11, 2023

I agree with the rule, but as with Callbacks Order, I think it's sufficient to give a small example (e.g. two filters) and have a link to the full list (e.g. the post referenced above).

@ydakuka
Copy link
Author

ydakuka commented Dec 11, 2023

have a link to the full list (e.g. the post referenced above).

I couldn't find this information, specifically the list, in the official Rails guide.

I think it's sufficient to give a small example (e.g. two filters) and have a link to the full list (e.g. the post referenced above).

Done.


[source,ruby]
----
# bad
Copy link
Member

Choose a reason for hiding this comment

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

This feels unnecessary

Copy link
Author

Choose a reason for hiding this comment

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

All bad examples?

README.adoc Outdated
around_action :around_action_filter1
around_action :around_action_filter2
after_action :after_action_filter
append_before_action :append_before_action_filter
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to use an append/prepend declarations in the same class as with regular before/after?

Copy link
Author

@ydakuka ydakuka Dec 11, 2023

Choose a reason for hiding this comment

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

Is there a reason to use an append/prepend declarations in the same class as with regular before/after?

Yes, I use them in my project, especially when working with built-in action filters from gems (for example, to run recaptcha checks before devise's built-in action filters). Otherwise, I use action filter without prepend_.

README.adoc Outdated
append_before_action :append_before_action_filter
append_around_action :append_around_action_filter
append_after_action :append_after_action_filter
prepend_before_action :prepend_before_action_filter
Copy link
Member

Choose a reason for hiding this comment

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

Aren’t prepend before or append after just aliases of before/after just like rspec hooks?

Copy link
Author

@ydakuka ydakuka Dec 11, 2023

Choose a reason for hiding this comment

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

@@ -266,6 +266,28 @@ class UsersController < ApplicationController
end
----

=== Action Filters Order [[action-filters-order]]

Order controller filter declarations in the order in which they will be executed.
Copy link
Member

Choose a reason for hiding this comment

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

This is getting complicated with inheritance and concerns.
When it comes to a point where order matters, isn’t it a sign that a different approach is needed?

Copy link
Author

Choose a reason for hiding this comment

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

I don't think that the action filters order will significantly impact the complexity of the code. Instead, it provides clarity in understanding the order in which the action filters are executed.

I try to avoid inheriting controllers other than inheriting from the application controller.
And as for the concerns, I haven't had any problems with them other than this one.

@pirj
Copy link
Member

pirj commented Dec 12, 2023

We have a related guideline https://github.com/rubocop/rails-style-guide#lexically-scoped-action-filter
But it doesn’t tell not to use filters in inherited classes or concerns.
I believe that complex inheritance chain is sometimes a necessity.

The consequence on our case is that the order of those filters is hard to predict. And ordering just local ones would just solve part of this disorder.

@ydakuka
Copy link
Author

ydakuka commented Dec 12, 2023

And ordering just local ones would just solve part of this disorder.

We would have the "correct" order in all controllers. And even with inherited controllers or concerns, the code would still be easier to read. At least it definitely won’t be more difficult.

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