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

Ignore C812 when only one function argument #59

Open
mlenzen opened this issue Dec 4, 2019 · 10 comments
Open

Ignore C812 when only one function argument #59

mlenzen opened this issue Dec 4, 2019 · 10 comments

Comments

@mlenzen
Copy link

mlenzen commented Dec 4, 2019

I'd like to ignore the error when there's only one argument to the function. For example:

logger.debug(
    "Really long string "
    "implicitly joined"
)
@sanjioh
Copy link

sanjioh commented Jan 9, 2020

+1

@shaleh
Copy link
Contributor

shaleh commented Jan 17, 2020

Why? The purpose of the trailing comma here is to cut down on diff noise in the future. It is totally plausible that some code passes one parameter when first written and then 3 in the iteration.

@9thbit
Copy link

9thbit commented Feb 21, 2020

I am all for trailing commas to reduce diff noise, thank you for developing this plugin! 😍 When we were trying to bring it into our flake8 setup, its flagging some cases similar to the above which I would not expect. For example when building some Django ORM queries we have code like:

intersting_pks = set(
    model.foreign_models
    .filter(some_criteria=True)
    .values_list("pk", flat=True)  # flags C812
)

I realise it would be valid syntax (in 3.7 at least) to have the , there at the end, but it just looks odd. Additionally, it does not really prevent diff churn/noise since it would not be valid to add another entry on the line following the comma.

Do you think it is feasible for the linter to allow for such syntax without having to add noqa or ignore C812? Happy to help contribute to such a change if you think it is reasonable.

@shaleh
Copy link
Contributor

shaleh commented Feb 21, 2020

What would the algorithm be @9thbit ? Because in your example if I replace set with an arbitrary function it becomes much less obvious that a comma does not make sense there.

BTW: I am not involved in this project, only another happy user. It is my hope that asking questions here will lead to the right outcome.

@9thbit
Copy link

9thbit commented Feb 21, 2020

Absolutely, or even removing set there would be justification for both with and without a the comma…
To be honest I'm not sure I'm not familiar with how this is checked currently or what the solution is. I would probably only expect trailing commas in things that are "list"-like, maybe is it just as simple as if statements on previous lines have trailing commas – that the same pattern should be maintained for the last item. 🤔

@shaleh
Copy link
Contributor

shaleh commented Feb 21, 2020

Give the code a read, it is fairly clear I think.

Right now there is minimal state gathering. I could see a more complicated version that differentiated between one argument functions and multi argument functions. Then you could ignore the one argument error if you liked. That is one solution. I am not sure at the moment how to determine the number of arguments a function requires but it should be possible to look at the parse tree and see how many are being passed in currently.

A place where the current checker does something weird is requiring a comma after "kwargs" which is definitely not going to see more arguments after it in most code.

@graingert
Copy link
Member

A place where the current checker does something weird is requiring a comma after "kwargs" which is definitely not going to see more arguments after it in most code.

black also requires a trailing comma after **kwargs (in py3.6+ only)

@SwampFalc
Copy link

My opinion on this matter is that it's not about differentiating between 1 or more arguments, but more about the difference between fixed or changing argument counts.

If a function call requires two arguments, period, then I almost feel like the trailing comma hints at a possibility of giving more. But doing so would cause an immediate error, which makes me feel like a trailing comma should be at least a warning and certainly not a recommendation.

Yes, of course, the function signature could some day change allowing (maybe even requiring) one more argument. But I feel that this would be such a profound impact that reducing the diff noise by one line is probably not going to matter...

@Sxderp
Copy link

Sxderp commented Dec 27, 2020

While I like this, I have also started using a "workaround" that also helps identify the string (or long bit) as a single argument. I'd much rather see #58 implemented.

The below doesn't throw any errors. It works out nicely for "segregating" arguments. I believe it also works for implicit concatenation, but I don't use that in my code.

logger.debug(
    (
        "Really long string " +
        "explicitly joined"
    ),
)

@ghost
Copy link

ghost commented Sep 25, 2021

I agree, this would be nice. There are cases where you know it's incredibly unlikely that a function is going to get another argument, so it just looks a bit ugly there.

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

7 participants