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

Implement PartialEq for http::Status with derive macro #2844

Closed
2 tasks done
frondeus opened this issue Aug 12, 2024 · 4 comments · Fixed by #2850
Closed
2 tasks done

Implement PartialEq for http::Status with derive macro #2844

frondeus opened this issue Aug 12, 2024 · 4 comments · Fixed by #2850
Labels
suggestion A suggestion to change functionality

Comments

@frondeus
Copy link
Contributor

API Docs to Existing Functionality

https://api.rocket.rs/v0.5/rocket/http/struct.Status

Problems with Existing Functionality

Status does not implement PartialEq via derive(PartialEq), which causes it to lack StructuralPartialEq.

It prohibits using status in match:

match status {
    Status::Unauthorized => { ... },
    Status::NotFound => { ... },
    _ => { ... }
}

because Status::Unauthorized is a constant which triggers error:

error: to use a constant of type `rocket::http::Status` in a pattern, `rocket::http::Status` must be annotated with `#[derive(PartialEq)]`
  --> my_project/src/errors.rs:40:13
   |
40 |             Status::Unauthorized => { ... },
   |             ^^^^^^^^^^^^^^^^^^^^
   |
   = note: the traits must be derived, manual `impl`s are not sufficient
   = note: see https://doc.rust-lang.org/stable/std/marker/trait.StructuralPartialEq.html for details

Suggested Changes

I've looked at current implementation of PartialEq for Status:
https://github.com/rwf2/Rocket/blob/1a3ef5b23fdf4a4619fa9b5d3f24cff6f8008085/core/http/src/status.rs#L363C1-L367C2

And it seems that there is nothing prohibiting automatic derive.

Alternatives Considered

An alternative solution to my match expression is something like this:

match status {
    status if status == Status::Unauthorized => { ... },
    status if status == Status::TooManyRequests => { ... },
}

But it doesn't feel idiomatic.

Another would be to use code directly:

match status.code {
    401 => { ... },
    429 => { ... }
}

But here I'm more prone to the typo than using existing constant.

Additional Context

No response

System Checks

  • I do not believe that this suggestion can or should be implemented outside of Rocket.

  • I was unable to find a previous suggestion for this change.

@frondeus frondeus added the suggestion A suggestion to change functionality label Aug 12, 2024
@SergioBenitez
Copy link
Member

Yes, we should do this. This is an artifact of Status originally having had a second "reason" field, which prevented using the derives. This field was later removed. If you could submit a PR, that would be helpful.

@the10thWiz
Copy link
Collaborator

As an addendum, we should also derive Eq, PartialOrd and Ord, for the same reason.

@frondeus
Copy link
Contributor Author

I'm eager to do the PR, should I also include Hash? It also seems to be a good candidate for derive.

@the10thWiz
Copy link
Collaborator

Yeah, might as well.

frondeus added a commit to frondeus/Rocket that referenced this issue Aug 19, 2024
# Main Reasoning

`PartialEq` when not implemented via derive macro, lacks
`StructuralPartialEq`. 

It causes undesired side effect, that the compiler rejects 
constants like `Status::Unauthorized` to be used in pattern 
(for example in the `match` expression)

# Solution

Because the current manual implementation is trivial and matches
derived code 1:1, we can replace it with `#[derive]` without
changes in the behaviour

Fixes rwf2#2844
SergioBenitez pushed a commit to frondeus/Rocket that referenced this issue Aug 19, 2024
`PartialEq` when not derived results in `StructuralPartialEq` not being
implemented. As this was the case for `http::Status`, matching against
constants like `Status::Unauthorized` was not allowed.

This commit replaces the manual implementations of equality traits
(`PartialEq`, `Eq`) and ordering traits (`PartialOrd`, `Ord`) for
`http::Status` with `#[derive]`.

Resolves rwf2#2844.
SergioBenitez pushed a commit to frondeus/Rocket that referenced this issue Aug 19, 2024
`PartialEq` when not derived results in `StructuralPartialEq` not being
implemented. As this was the case for `http::Status`, matching against
constants like `Status::Unauthorized` was not allowed.

This commit replaces the manual implementations of equality traits
(`PartialEq`, `Eq`) and ordering traits (`PartialOrd`, `Ord`) for
`http::Status` with `#[derive]`.

Resolves rwf2#2844.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
suggestion A suggestion to change functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants