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

treewide: silence mock structs unused warning #996

Merged

Conversation

wprzytula
Copy link
Collaborator

@wprzytula wprzytula commented May 10, 2024

With a new version of Rust toolchain (stable 1.78.0), our never constructed structs used for testing macros began to trigger rustc warnings. Now, they are silenced explicitly using #[allow(unused)].

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • [ ] I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • [ ] I have provided docstrings for the public items that I want to introduce.
  • [ ] I have adjusted the documentation in ./docs/source/.
  • [ ] I added appropriate Fixes: annotations to PR description.

With a new version of Rust toolchain, our never constructed structs used
for testing macros began to trigger rustc warnings. They are silenced
explicitly using #[allow(unused)].
@wprzytula wprzytula requested a review from Lorak-mmk May 10, 2024 05:33
@wprzytula
Copy link
Collaborator Author

If considerably many users are already using the new toolchain, we may want to consider releasing a patch version to silence those warnings.

@wprzytula wprzytula added this to the 0.14.0 milestone May 10, 2024
Copy link

cargo semver-checks found no API-breaking changes in this PR! 🎉🥳
Checked commit: 3455abf

@wprzytula wprzytula requested a review from muzarski May 10, 2024 08:00
@wprzytula wprzytula self-assigned this May 14, 2024
@Lorak-mmk
Copy link
Collaborator

If considerably many users are already using the new toolchain, we may want to consider releasing a patch version to silence those warnings.

I see only changes in tests, it should not affect users of the driver so I don't see the point at all.

When are the warnings fixed by this PR triggered? I didn't see any failed CI runs because of this.
Also, instead of saying new version of Rust toolchain please specify the version - someone reading this in the future won't know what version you had in mind without checking release dates.

@wprzytula
Copy link
Collaborator Author

I see only changes in tests, it should not affect users of the driver so I don't see the point at all.

Agreed.

When are the warnings fixed by this PR triggered? I didn't see any failed CI runs because of this.

They pop up when I run cargo check --all-targets.

Also, instead of saying new version of Rust toolchain please specify the version - someone reading this in the future won't know what version you had in mind without checking release dates.

Done.

@Lorak-mmk
Copy link
Collaborator

When are the warnings fixed by this PR triggered? I didn't see any failed CI runs because of this.

They pop up when I run cargo check --all-targets.

Then why is CI not failing on other PRs?

@Lorak-mmk
Copy link
Collaborator

Ah, it's probably not failing because we use Rust provided on CI runner images instead of setting up our own and I guess they were not updated yet.
We should probably change this at some point to use latest stable always.

@wprzytula wprzytula merged commit c25c54d into scylladb:main May 14, 2024
12 checks passed
@wprzytula wprzytula deleted the silence-mock-structs-never-constructed branch May 14, 2024 11:37
@Lorak-mmk
Copy link
Collaborator

Eh, I forgot to check the commit message and you use new version of Rust toolchain there too.
Well, it's too late now and it's not that important

wprzytula added a commit to wprzytula/scylla-rust-driver that referenced this pull request Jul 11, 2024
…ever-constructed

treewide: silence mock structs unused warning
(cherry picked from commit c25c54d)
@wprzytula wprzytula mentioned this pull request Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants