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

clippy: apply missing_const_for_fn lint #523

Closed
wants to merge 4 commits into from

Conversation

CosminPerRam
Copy link
Contributor

@CosminPerRam CosminPerRam commented Apr 29, 2024

This PR applies the missing_const_for_fn lint and also adds it to the CI.

Technically the majority of them should be already optimized away, but if they can be const, should they be const?

I also found 3 instances of translations functions having per item .to_string() instead of on the entire match block and a single translation function call that was cloned unnecessarily.

@GyulyVGC GyulyVGC added the enhancement New feature, request, or improvement label Apr 29, 2024
@GyulyVGC
Copy link
Owner

As mentioned in the same lint page you linked:

Const functions are currently still being worked on, with some features only being available on nightly. This lint does not consider all edge cases currently and the suggestions may be incorrect if you are using this lint on stable.

In general I think that's better to rely on the lints included by default in clippy warnings, since they are the ones considered to be more stable and impactful.

I'm also used to run quite often clippy pedantic, and not even that includes this specific lint.

I prefer waiting until this is stabilised.

Thanks anyway for your effort!

@GyulyVGC GyulyVGC closed this Apr 29, 2024
@CosminPerRam
Copy link
Contributor Author

CosminPerRam commented Apr 29, 2024

Well, we don't have to add that lint to CI, but other than that, the changes are there to be added.

@GyulyVGC
Copy link
Owner

Yeah, as you say I don't think it'd be an issue to merge these changes, but since the official docs themselves warn that suggestions may be incorrect, I prefer waiting the stabilisation of the lint to be sure to apply it only where really necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature, request, or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants