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

feat(is_variant): add #[must_use] annotation #350

Merged
merged 10 commits into from
Apr 19, 2024

Conversation

TheLostLambda
Copy link
Contributor

@TheLostLambda TheLostLambda commented Apr 18, 2024

Resolves #349

Synopsis & Solution

Adds a #[must_use] annotation to the static methods generated by IsVariant. I also refactored the derive to use matches!(...) instead of a manual match block with bools, but that's just a style tweak.

Checklist

  • Documentation is updated (if required)
  • Tests are added/updated (if required)

I'll be honest, I'm not 100% sure how to write tests for this one, but I have a screenshot of it in action:
Screenshot from 2024-04-18 10-32-13

Copy link
Collaborator

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

@TheLostLambda thanks for quick reaction!

Let's add few bikesheddings and get it merged!

#enum_name ::#variant_ident #data_pattern => true,
_ => false
}
matches!(self, #enum_name ::#variant_ident #data_pattern)
Copy link
Collaborator

Choose a reason for hiding this comment

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

For hygiene, this should be used like derive_more::core::matches!.

impl/doc/is_variant.md Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
@tyranron tyranron added this to the 1.0.0 milestone Apr 18, 2024
@tyranron
Copy link
Collaborator

tyranron commented Apr 18, 2024

I'll be honest, I'm not 100% sure how to write tests for this one, but I have a screenshot of it in action:
Screenshot from 2024-04-18 10-32-13

You can add tests/compile_fail with #[forbid(unused_must_use)]. So if this will be removed in future, the test case will compile successfully, and so, fail the test.

@TheLostLambda
Copy link
Contributor Author

@tyranron Ah! Excellent! The tests/compile_fail is exactly what I was looking for!

I'll get to work addressing these loose ends!

@TheLostLambda
Copy link
Contributor Author

TheLostLambda commented Apr 18, 2024

I just went a little insane trying to figure out why those tests weren't running for me, but it seems the compile fail tests are all disabled on nightly Rust (only running for stable).

I think that means all of these tests are missed by CI as well, since there are a couple of failing tests unrelated to my change here.

Would you like me to add something to the CI in this PR or remove that stable-only attribute? I assume that was added in the first place to stop things from breaking when the nightly and stable compilers gave different errors, so perhaps just adding a bit in the CI is the best thing to do?

EDIT: Actually, they look like they should be running? Let me investigate...

@TheLostLambda
Copy link
Contributor Author

Conclusion is that I have no idea why that last CI run didn't fail... It looks like things are being run in the stable test CI (one of the matrix options), but it passed in CI and failed for me, with exactly the same version of stable Rust?

I've pushed the changes that make things pass for me locally, so maybe those will break the CI version? They should?

TL;DR is that I think I've addressed all of your comments, but, unrelated, I'm not sure if the compile_fail tests are working correctly either in CI or on my machine...

Copy link
Collaborator

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

@TheLostLambda this is just some platform-dependent output weirdness.

Overall, now it's LGTM. Thanks!

@tyranron tyranron merged commit e0d1698 into JelteF:master Apr 19, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IsVariant should annotate the generated methods with #[must_use]?
2 participants