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

Make web::Path innards public #3588

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

IriaSomobu
Copy link

@IriaSomobu IriaSomobu commented Feb 27, 2025

PR Type

Other?

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

This change allows us to do something like this:

async fn delete_channel_message(
    web::Path((channel_id, message_id)): web::Path<(Snowflake, Snowflake)>,
) {
  // here channel_id and message_id are Snowflakes,
  // and not web::Path<Snowflake>
}

Dunno why other web types are public and this is not.

So we can do something like this:
```
async fn delete_channel_message(
    web::Path((channel_id, message_id)): web::Path<(Snowflake, Snowflake)>,
) {
  // here channel_id and message_id are Snowflakes,
  // and not web::Path<Snowflake>
}
```

Dunno why other web types are public and this is not.
@robjtede robjtede added B-semver-major breaking change requiring a major version bump A-web project: actix-web labels Feb 27, 2025
@robjtede robjtede added this to the actix-web v5.0 milestone Feb 27, 2025
@robjtede
Copy link
Member

This field was made private for v4.0 but later it was realized this was the wrong trade-off. Making it public again is certainly planned for v5.0.

This is a breaking change due to the Deref impl, so it should also be removed in this PR.

For now, you can use the Path type from actix-web-lab to get destructuring.

@IriaSomobu
Copy link
Author

IriaSomobu commented Feb 27, 2025

This is a breaking change due to the Deref impl, so it should also be removed in this PR.

Could you elaborate please?

@robjtede
Copy link
Member

The thing we were seeing in v3.x was users reporting confusion using this type without destructuring because the Deref impl allowed access to tuple elements like path.1 but then path.0.1 also worked so it was weird. Anyway, making this public will make the path.0 case ambiguous to the compiler in certain cases, making this a breaking change.

@IriaSomobu
Copy link
Author

Okey, I get it. path.0.1 really looks funny. Gimme 5min -- I'll remove that deref.

@robjtede
Copy link
Member

No rush. Like I say, this will need to wait for v5.0.

@robjtede: Deref impl [with public Path contents] allowed access to tuple elements like path.1 but then path.0.1 also worked so it was weird.
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-major breaking change requiring a major version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants