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 all items are on one line #58

Open
mlenzen opened this issue Oct 7, 2019 · 7 comments
Open

Ignore C812 when all items are on one line #58

mlenzen opened this issue Oct 7, 2019 · 7 comments

Comments

@mlenzen
Copy link

mlenzen commented Oct 7, 2019

I'd like to ignore the enforcement of a trailing comma where all items are on a single line that isn't the same line as the opening bracket. For example,

long_function_name(
    argument1, argument2, keyword=argument3
)
@vpoulailleau
Copy link

I agree! Especially, https://github.com/psf/black formats the code like this for "short" lists of function parameters.

@sanjioh
Copy link

sanjioh commented Jan 9, 2020

Strong +1 on this :)

@PeterJCLaw
Copy link
Contributor

Serious question: if you like that format, without the comma, and are already using black, which formats your code, then why are you using flake8-commas?

@vpoulailleau
Copy link

Not for me, but for other coders (especially newcomers)

@mlenzen
Copy link
Author

mlenzen commented Oct 2, 2020

I personally use black but we don't enforce it's use by all developers. The linting rules are our standard.

@gorrog
Copy link

gorrog commented Nov 27, 2020

If this issue is still being considered, I'd like to add this useful SO discussion to the debate in favour of this issue.

https://stackoverflow.com/questions/12087742/should-i-add-a-trailing-comma-after-the-last-argument-in-a-function-call

In particular, this bit:

Conclusion:

Recommended in multi-line data structures that are expected to physically change
Rarely to never in function calls
Never in function definitions

@PeterJCLaw
Copy link
Contributor

@gorrog thanks for adding that, it's interesting to note the differences between this package and PEP8 as described in that post. While I think generally PEP8 is/was a good starting point for formatting Python, a lot has been learned over the years since it was created. As a result, I think it is reasonable that we allow linters and other tools to implement nuanced improvements over the guidelines it sets out. Additionally: PEP8 already existed when this project was created, so I don't find the mismatch between it and PEP8 a compelling argument to change the direction it takes.


Currently my personal preference in this situation is that this linter force the presence of a comma. This is substantially because it forces black to wrap the function to the longer format, which then improves consistency & reduces diff noise1.


(back to speaking as a maintainer)

I'm not really sure I understand the desire to use this linter alongside black without this rule, though I suspect I'm missing something about the cases described which makes it work for others.

Here's my current understanding & reasoning -- please do let me know where I'm missing why a case is useful!

  1. the codebase uses black, but also wants to use flake8-commas -- why bother when black (I think) supersedes flake8-commas in all other cases? (I think this miiight be the case @vpoulailleau is describing, though I'm not sure, and I feel I'm missing the value in presenting errors to newcomers about commas when black will fix them automatically)
  2. the codebase uses black, but an individual wants to use flake8-commas -- this feels like a recipe for disagreement across the rules (it just so happens that the other rules line up)
  3. the codebase uses flake8-commas but also wants to use black -- this is kinda the same as the first case, but in reverse, as with that case I'm struggling to see why using both would be useful
  4. the codebase uses flake8-commas but an individual wants to use black -- this one I can maybe see some mileage in (and what I think @mlenzen is describing), however if the codebase already uses flake8-commas then surely the codebase is expecting the trailing commas here (and is using flake8-commas to lint that) and thus changing the tool feels like an oblique way to change the codebase's style guide?

I'd love to know what I've missed/misunderstood here!

I might be open to a PR which changed this case to being a separate error code (but not to remove it completely), as long as doing so didn't look like it would present too much maintenance burden (probably and wasn't too invasive into the rest of the code). We would likely also need to coordinate such a change with the ruff folks, who also have their own formatter interactions to also consider (astral-sh/ruff#9216).

Footnotes

  1. I find it somewhat ironic that black claims to reduce diff noise and increase consistency, yet has that intermediate wrapping style which runs counter to both of those goals.

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

5 participants