Skip to content

Add ConditionOption middleware#2858

Open
KoltesDigital wants to merge 1 commit into
actix:mainfrom
KoltesDigital:condition-option
Open

Add ConditionOption middleware#2858
KoltesDigital wants to merge 1 commit into
actix:mainfrom
KoltesDigital:condition-option

Conversation

@KoltesDigital

@KoltesDigital KoltesDigital commented Aug 31, 2022

Copy link
Copy Markdown

PR Type

Feature

PR Checklist

  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • A changelog entry has been made for the appropriate packages.
  • Format code with the latest stable rustfmt.
  • (Team) Label with affected crates and semver status.

Overview

I'm interested in conditionally applying a middleware. But with Condition, I find strange that the middleware has to be created even if the condition is false.

One simple, quite idiomatic improvement would have been to use a lambda to lazily generate the middleware.

But there's a more idiomatic way to do, using Option!

As Transform is defined in another crate, a newtype is required, which I called ConditionOption. Sadly it has to be mentioned for wrap to work.

@fakeshadow

Copy link
Copy Markdown
Contributor

This should be added to actix-service where Transform can impl Option

@KoltesDigital

Copy link
Copy Markdown
Author

That's what I thought at the beginning, then I saw that the latest documentation for Transform mentions impls for Arc and Rc but they've disappeared from the master branch, so I thought that its impls have to be removed.

In the other hand, that would move ConditionMiddleware as well. I don't master your architecture, I can't make such a bold move.

@fakeshadow

Copy link
Copy Markdown
Contributor

You can start by making an issue in actix-net repo. We want to get it right since we already know what shoud be done. Rather than just add more duplicated (purpose) code.

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

Labels

A-web project: actix-web B-semver-minor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants