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

How to deal with #[warn(clippy::too_many_arguments)] #44

Closed
igalic opened this issue Mar 13, 2019 · 7 comments
Closed

How to deal with #[warn(clippy::too_many_arguments)] #44

igalic opened this issue Mar 13, 2019 · 7 comments

Comments

@igalic
Copy link

igalic commented Mar 13, 2019

We've recently started a PR to clippy-fy our code Plume-org/Plume#462 and we're hitting some limits in Ructe, with regard to how templates are generated as functions

in our case, as functions with lots of parameters, despite having extracted a BaseContext

Here's an example of such a function: https://github.com/Plume-org/Plume/blob/master/templates/posts/details.rs.html#L12

@kaj
Copy link
Owner

kaj commented Mar 14, 2019

Wow, that's a lot of parameters! But even with fewer parameters, I have seen the same problem.

So, what should we do? Just add #[allow(clippy::too_many_arguments)] to all generated templates? Or can we say that the the template module has a limit of e.g. 14 arguments instead of the default 7 ?

@kaj
Copy link
Owner

kaj commented Mar 14, 2019

https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments says that there is a configuration too-many-arguments-threshold: u64, but I don't know if it can be configured in a module. On the other hand, e.g. nrc/derive-new#13 (comment) may apply to us, maybe for templats with lots of arguments, some arguments should be grouped so there is fewer arguments total?

Another approach may be to simply set too-many-arguments-threshold to a larger value in the lint configuration of a project that uses templates with many argumens?

@igalic
Copy link
Author

igalic commented Mar 14, 2019

i think the problem in our case is that the module in question is src/main.rs: https://github.com/Plume-org/Plume/blob/42dca3daaee3e647337ceb7e18ab97b8a133a95e/src/main.rs#L62-L66

A submodule thereof is routes, where i've done some of our finest work thanks to the too_many_arguments lint! Plume-org/Plume@965e97c

@kaj
Copy link
Owner

kaj commented Mar 14, 2019

I just verified that this workaround works for me:

mod foo {
    #![allow(clippy::too_many_arguments)]
    include!(concat!(env!("OUT_DIR"), "/templates.rs"));
}
use foo::templates;

A bit ugly, but might be worthwile while we try to figure out a better way to handle things.

@kaj
Copy link
Owner

kaj commented Aug 24, 2019

Seems ok to close this. I'll just throw in a note that the dummy module in the above-mentioned workaround don't seem to be needed any more (I don't know why it was needed beore). So the "workaround" can be reduced to:

#[allow(clippy::too_many_arguments)]
include!(concat!(env!("OUT_DIR"), "/templates.rs"));

... which doesn't even feel like a workaround, but rather proper clippy configuration.

@kaj kaj closed this as completed Aug 24, 2019
@vbrandl
Copy link
Contributor

vbrandl commented Dec 14, 2021

I have a kinda similar problem in vbrandl/hoc so I don't know if this requires a new issue, but I think it fits here:

warning: this expression borrows a reference (`&mut W`) that is immediately dereferenced by the compiler
  --> /home/me/code/hoc/target/debug/build/hoc-7df636231525c575/out/templates/template_p500_html.rs:10:6
   |
10 | base(&mut _ructe_out_, "Internal Server Error - Hits-of-Code Badges", "500 - Internal Server Error", |mut _ructe_out_| {
   |      ^^^^^^^^^^^^^^^^ help: change this to: `_ructe_out_`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow

I tried working around that lint:

mod workaround {
    #[allow(clippy::needless_borrow)]
    include!(concat!(env!("OUT_DIR"), "/templates.rs"));
}
use workaround::templates;

But this doesn't seem to work anymore:

warning: unused attribute `allow`
  --> src/lib.rs:52:5
   |
52 |     #[allow(clippy::needless_borrow)]
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(unused_attributes)]` on by default
note: the built-in attribute `allow` will be ignored, since it's applied to the macro invocation `include`
  --> src/lib.rs:53:5
   |
53 |     include!(concat!(env!("OUT_DIR"), "/templates.rs"));
   |     ^^^^^^^

Is there another way to suppress clippy lints for ructe templates?

@kaj
Copy link
Owner

kaj commented Dec 14, 2021

That is separate, and a lint that started to appear on all ructe templates witch rustc / clippy version 1.57. I'm thinking of changing the generated signature to fix it, perhaps as tried in #107 , perhaps in some slightly different way.

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

No branches or pull requests

3 participants