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

Fix trailing comma for function with one arg (#880) #891

Conversation

djb7
Copy link
Contributor

@djb7 djb7 commented Jun 9, 2019

Modified maybe_remove_trailing_comma to remove trailing commas for typedarglists (in addition to arglists), and updated line split logic to ensure that all lines in a function definition that contain only one arg have a trailing comma.

Some discussion points:

  • There are likely some edge cases to consider with the use of no_commas. Would be great to hear any other suggestions for this test.
  • The new test data file should probably be folded into one of the existing function test files, are there any guidelines around this?
  • It looks like this will clash with Pull request tweak collection literals to explode with trailing comma #826.

@djb7 djb7 force-pushed the bugfix/880-trailing-comma-for-function-with-one-arg branch from 781b6a2 to c7943f2 Compare June 9, 2019 07:18
@ghost
Copy link

ghost commented Jun 9, 2019

(ref. issue #880 )

black.py Outdated
if original.is_import:
# Ensure a trailing comma for imports and standalone function arguments, but
# be careful not to add one after any comments.
no_commas = original.is_def and not [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use any()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, cheers.

@@ -264,6 +264,14 @@ def test_function2(self) -> None:
black.assert_equivalent(source, actual)
black.assert_stable(source, actual, black.FileMode())

@patch("black.dump_to_file", dump_to_stderr)
def test_function3(self) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine to have another test file, but could you name it something less generic, like test_function_trailing_comma?

Modified maybe_remove_trailing_comma to remove trailing commas for
typedarglists (in addition to arglists), and updated line split logic
to ensure that all lines in a function definition that contain only one
arg have a trailing comma.
@djb7 djb7 force-pushed the bugfix/880-trailing-comma-for-function-with-one-arg branch from c7943f2 to 7e822f3 Compare June 10, 2019 07:39
Copy link
Collaborator

@zsol zsol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks for fixing this!

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

Successfully merging this pull request may close these issues.

None yet

3 participants