Skip to content

"Middleware#remove" is renamed "Middleware#delete!" #42867

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

Merged
merged 5 commits into from
Jul 28, 2021

Conversation

sato11
Copy link
Contributor

@sato11 sato11 commented Jul 25, 2021

Summary

This commit intends to clarify the difference between
Middleware#delete and Middleware#delete!.

The former method silently fails when the target item is
not found, while the latter raises an error.

The functionality of delete! has been introduced in 688ed70
and given a name remove. This commit only renames it.

Also, a brief description of delete! method is now
provided for guides so that users can acknowledge the difference.

Resolves #42862

This commit intends to clarify the difference between
`Middleware#delete` and `Middleware#delete!`.

The former method silently fails when the target item is
not found, while the latter raises an error.

The functionality of `delete!` has been introduced in 688ed70
and given a name `remove`. This commit only renames it.

Also, a brief description of `delete!` method is now
provided for guides so that users can acknowledge the difference.
@@ -133,7 +133,7 @@ def delete(target)
middlewares.reject! { |m| m.name == target.name }
end

def remove(target)
def delete!(target)
Copy link
Member

Choose a reason for hiding this comment

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

Could you also add some rdocs for delete! and delete?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure I will! 😄

@sato11
Copy link
Contributor Author

sato11 commented Jul 26, 2021

@marivaldo @p8
With suggested changes have been made, I'm leaving these commits unsquashed, not knowing if it's good or bad. I'm flexible and ready to meld them into one however, so let me know if you guys have an idea about it 🙂

@sato11 sato11 requested a review from p8 July 26, 2021 00:44
sato11 and others added 2 commits July 26, 2021 17:55
Co-authored-by: Petrik de Heus <petrik@deheus.net>
Co-authored-by: Petrik de Heus <petrik@deheus.net>
@sato11 sato11 requested a review from p8 July 26, 2021 09:09
@p8
Copy link
Member

p8 commented Jul 26, 2021

@morgoth what do you think?

@morgoth
Copy link
Member

morgoth commented Jul 26, 2021

@p8 I find it much more clear. 👍

@p8
Copy link
Member

p8 commented Jul 26, 2021

@morgoth Great! 🎉

@p8 p8 added the ready PRs ready to merge label Jul 26, 2021
@rafaelfranca rafaelfranca merged commit 9d2a214 into rails:main Jul 28, 2021
@sato11 sato11 deleted the rename-middleware-remove branch July 30, 2021 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
actionpack docs ready PRs ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify difference between Middleware#remove and Middleware#delete
5 participants