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 Hash for ParseError #14

Merged
merged 1 commit into from
Jul 20, 2024

Conversation

daxpedda
Copy link
Member

@daxpedda daxpedda commented Jul 17, 2024

This implements Hash for ParseError.
I assume that Clone and Copy wasn't added because potentially in the future some data might be added to ParseError?

I also replaced ParseError::_private with #[non_exhaustive], kinda the new Rusty way to do this. Let me know if you want me to split this into a separate PR.

Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

I'm not sure #[non_exhaustive] really works on empty structs? See this playground

@notgull
Copy link
Member

notgull commented Jul 17, 2024

I also replaced ParseError::_private with #[non_exhaustive], kinda the new Rusty way to do this. Let me know if you want me to split this into a separate PR.

The docs render differently and show an empty struct rather than a private struct. So I would prefer not to do this.

@daxpedda
Copy link
Member Author

daxpedda commented Jul 17, 2024

I'm not sure #[non_exhaustive] really works on empty structs? See this playground

It does work, just not on a intra-crate level, but on a inter-crate level.
You can try it in a local example.

I also replaced ParseError::_private with #[non_exhaustive], kinda the new Rusty way to do this. Let me know if you want me to split this into a separate PR.

The docs render differently and show an empty struct rather than a private struct. So I would prefer not to do this.

Looks like this:
image
I agree that's worse.

@daxpedda daxpedda force-pushed the parse-error-hash branch 2 times, most recently from 1af7fe6 to dc2400f Compare July 17, 2024 08:37
@daxpedda
Copy link
Member Author

Never seen this before:

#![cfg_attr(feature = "cargo-clippy", deny(warnings))]

What is this supposed to do and how exactly should this be fixed?

@daxpedda daxpedda requested a review from madsmtm July 17, 2024 08:38
@kchibisov
Copy link
Member

@daxpedda the point was to not allow building of the struct while remaining it public, probably it does achieve that, but I don't really see a value in changing the _private? Like it adds nothing really.

@daxpedda
Copy link
Member Author

@daxpedda the point was to not allow building of the struct while remaining it public, probably it does achieve that, but I don't really see a value in changing the _private? Like it adds nothing really.

I already reverted that because of @notgull's feedback.

What it would have done is to use the recommended way Rust achieves exactly this, instead of having to add a field that does nothing, which is basically a workaround. Most people consider adhering to common code practices a code quality improvement.

@kchibisov kchibisov merged commit 35d512d into rust-windowing:main Jul 20, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants