Remove FromRequest::Config

[arniu]

PR Type

Refactor

PR Checklist

• 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

closes #2221

[robjtede]

Aside from String extractor respecting PayloadConfig becoming non-obvious with this change, I don't have a strong opinion and would like to hear from @actix/contributors.

[fakeshadow]

This is a breaking change for nothing.

[jplatte]

What was the point in the first place if this associated type isn't actually needed (as #2221 says)?

Aside from String extractor respecting PayloadConfig becoming non-obvious with this change, …

IMHO "respects some server config" should just be plain documentation (as it already is).

[ibraheemdev]

I like this change, it simplifies the public API.

[fakeshadow]

Arguing it's needed or not is pointless. You should do that when the trait is first introduced.

The point of having it is people don't have to make break change. It's a long standing trait and it works fine and that's all.

[ibraheemdev]

Yeah, it's obvious that the type is not needed. I guess the decision is just whether or not it's worth a breaking change. You could argue both ways.

[popzxc]

You should do that when the trait is first introduced.

IMHO it doesn't mean that things may never be improved. Having breaking changes is OK, that's why we have semver in the first place.
In that case, the migration procedure is simple, and it actually improves the interface: having less associated types/generic parameters is usually better (and in that case, it is better).

I personally like this change.

[fakeshadow]

You should do that when the trait is first introduced.

IMHO it doesn't mean that things may never be improved. Having breaking changes is OK, that's why we have semver in the first place.
In that case, the migration procedure is simple, and it actually improves the interface: having less associated types/generic parameters is usually better (and in that case, it is better).

I personally like this change.

What improvement? Reduce one loc when you importing the trait? Does it really make a difference?
I would rather people actually export this trait and expose it in their apps/libs not be breaking for this meaningless change.

You have to remember any lib using this trait would be affected and they need a new release just for this change alone.

Look at the current betas. You already have enough people can not figuring out how to make actix-xxx be compat with their beta releases.

[neoeinstein]

Having this coupled with a SemVer major release makes this the right time to make this change. It's vestigial and confusing to consumers. After the change is made, the API will be cleaner and the better for it.

[popzxc]

What improvement? Reduce one loc when you importing the trait? Does it really make a difference?

I'd say that it's not about writing/removing one line of code, but rather figuring out what the heck is it and do you actually need it (spoiler: you don't).

The point of upgrades is not only to care about users who already use the library but also to care about ones who will use it in the future. Giving the fact that Rust's popularity constantly grows and actix-web is a pretty important framework for its infrastructure, I think that simplifying things is always a good thing.

Otherwise, we can get stuck forever with "it works for current users, they already know how it works and there is no need to introduce the inconvenience".

[fakeshadow]

There are so many dead code in current code base and you are like nah they are fine and I want this public API change for nothing to happen.
I already said in previous reply it's pointless to argue if it's need or not. You can remove what you need or keep what you don't. It all comes down to what you actually gain from the change and this is the hard fact of this PR.

You save one loc for using this trait at the cost of breaking a public API.

I won't make further reply in this thread as there is no point to argue for nothing. This PR does nothing break something and real changes for meaningful PRs are ignored. We should put effort on those.

[popzxc]

There are so many dead code in current code base

It also doesn't sound good to me, to be honest. I think it's a good idea to remove all the dead code, because why keep it.

[arniu]

Yes, the change is trivial, and it is a break change.

But, it make the public API cleaner and simple and it does make sense.

Now we are marching toward a new, groundbreaking version.

It's time to make some break change!

[robjtede]

I'm not entirely convinced about this yet. If it is going to be merged I'd like to see adequate docs for String and Bytes extractors regarding their config type.

[popzxc]

I'd like to see adequate docs for String and Bytes extractors regarding their config type.

👍 on this. Also, it will be really nice to extend FromRequest trait docs with information on how you can configure extractor, ideally with a doc-test example. Like, while most users don't need it, it'd be helpful for ones who in fact do.

[arniu]

I am not a native English speaker. Any one can help to improve the words?

[popzxc]

In general, I'm happy with the added docs. Great job.

[LukeMathWalker]

2c from a long-time actix-web user (+library author, both public and private): the simpler you can make traits that are used often by application users, the better. Each generic parameter/associated type adds a non-trivial amount of complexity, especially in the hands of non-very-experienced Rust users.
E.g. writing middlewares for actix-web is not easy at all.

If you have a chance to simplify without losing expressiveness or performance, I'd take it (of course providing good docs on how to move forward).

[arniu]

I really hope it can be merged before 4.0.0. And what's the problem for the moment?

[popzxc]

@robjtede ?

[omid]

OK, this breaks serde_qs: https://github.com/samscott89/serde_qs/blob/main/src/actix.rs#L100
What is the workaround to pass a config?

[jplatte]

@omid There is no workaround required. req.app_data::<QsQueryConfig>() will still work without the associated type.

[omid]

@jplatte thanks. So in this code here: https://github.com/samscott89/serde_qs/blob/main/src/actix.rs#L151
We need to just set app_data with the QsQueryConfig, right?

[jplatte]

I don't really understand what you mean or what the QsQuery::<Info>::configure in the docs you ilnked is about. I think it would be good to open a Q&A discussion for this rather than discussing on this merged PR.