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

too_many_arguments should ignore functions called new #1576

Open
sgrif opened this issue Feb 26, 2017 · 6 comments
Open

too_many_arguments should ignore functions called new #1576

sgrif opened this issue Feb 26, 2017 · 6 comments

Comments

@sgrif
Copy link

sgrif commented Feb 26, 2017

The recommendation provided by that lint is to group the arguments into a new type, and should probably ignore functions that are almost certainly a constructor for a new type.

@Arnavion
Copy link
Contributor

Arnavion commented Mar 1, 2017

Related: nrc/derive-new#13

@richard-uk1
Copy link

I'd argue that if there are that many fields, it makes sense to use a Builder, avoiding a single function call.

@llogiq
Copy link
Contributor

llogiq commented Aug 6, 2017

Perhaps we can special-case the lint message?

@workingjubilee
Copy link
Member

The Builder pattern is a way to gradually build data, it is not necessarily useful to increase abstraction when all the data is known and already present.

I believe this can be changed in the following match:

// don't warn for implementations, it's not their fault
if !is_trait_impl_item(cx, hir_id) {
// don't lint extern functions decls, it's not their fault either
match kind {
intravisit::FnKind::Method(
_,
&hir::FnSig {
header: hir::FnHeader { abi: Abi::Rust, .. },
..
},
_,
)
| intravisit::FnKind::ItemFn(_, _, hir::FnHeader { abi: Abi::Rust, .. }, _) => check_arg_number(
cx,
decl,
span.with_hi(decl.output.span().hi()),
too_many_arguments_threshold,
),
_ => {},

For the FnKind::Method variant we can check the Ident field to see if it is new, see:
https://doc.rust-lang.org/nightly/nightly-rustc/rustc_hir/intravisit/enum.FnKind.html

@richard-uk1
Copy link

I'd argue that if there are that many fields, it makes sense to use a Builder, avoiding a single function call.

4 years down the line, I'd now disagree with myself, and say that constructors are sometimes better than builders.

@camsteffen
Copy link
Contributor

I can see how it might be deemed necessary to have a lot of args for a constructor. But it is still a code smell generally speaking, the lint is not "wrong", and so I don't think a special case should be added. You can allow on the constructor. You could also consider refactoring by grouping some fields into structs.

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

6 participants