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

Standalone comments inside nested bracketed expressions block the expression from wrapping #22

Closed
njsmith opened this issue Mar 15, 2018 · 4 comments
Labels
F: comments The syntactic kind. Not in the language grammar, always on our minds. Best bugs. T: bug Something isn't working

Comments

@njsmith
Copy link

njsmith commented Mar 15, 2018

This is... not what I expected black to do :-)

     # Capture each of the exceptions in the MultiError along with each of their causes and contexts
     if isinstance(exc_value, MultiError):
         embedded = []
         for exc in exc_value.exceptions:
             if exc not in _seen:
                 embedded.append(
-                    traceback.TracebackException.from_exception(
-                        exc,
-                        limit=limit,
-                        lookup_lines=lookup_lines,
-                        capture_locals=capture_locals,
-                        # copy the set of _seen exceptions so that duplicates
-                        # shared between sub-exceptions are not omitted
-                        _seen=set(_seen)
-                    )
+                    traceback.TracebackException.from_exception(exc, limit=limit, lookup_lines=lookup_lines, capture_locals=capture_locals, _seen=set(_seen))
+                    # copy the set of _seen exceptions so that duplicates
+                    # shared between sub-exceptions are not omitted
                 )
@ambv ambv added T: bug Something isn't working F: comments The syntactic kind. Not in the language grammar, always on our minds. Best bugs. labels Mar 16, 2018
@dstufft
Copy link

dstufft commented Mar 22, 2018

I believe I am also experiencing this, which ends up with a 3000+ length line for me:

@@ -749,183 +667,19 @@ class TestFileUpload:

     @pytest.mark.parametrize(
         ("post_data", "message"),
-        [
-            # metadata_version errors.
-            (
-                {},
-                "None is an invalid value for Metadata-Version. "
-                "Error: This field is required. "
-                "see "
-                "https://packaging.python.org/specifications/core-metadata",
-            ),
-            (
-                {"metadata_version": "-1"},
-                "'-1' is an invalid value for Metadata-Version. "
-                "Error: Unknown Metadata Version "
-                "see "
-                "https://packaging.python.org/specifications/core-metadata",
-            ),
-
-            # name errors.
-            (
-                {"metadata_version": "1.2"},
-                "'' is an invalid value for Name. "
-                "Error: This field is required. "
-                "see "
-                "https://packaging.python.org/specifications/core-metadata",
-            ),
-            (
-                {"metadata_version": "1.2", "name": "foo-"},
-                "'foo-' is an invalid value for Name. "
-                "Error: Must start and end with a letter or numeral and "
-                "contain only ascii numeric and '.', '_' and '-'. "
-                "see "
-                "https://packaging.python.org/specifications/core-metadata",
-            ),
-
-            # version errors.
-            (
-                {"metadata_version": "1.2", "name": "example"},
-                "'' is an invalid value for Version. "
-                "Error: This field is required. "
-                "see "
-                "https://packaging.python.org/specifications/core-metadata",
-            ),
-            (
-                {
-                    "metadata_version": "1.2",
-                    "name": "example",
-                    "version": "dog",
-                },
-                "'dog' is an invalid value for Version. "
-                "Error: Must start and end with a letter or numeral and "
-                "contain only ascii numeric and '.', '_' and '-'. "
-                "see "
-                "https://packaging.python.org/specifications/core-metadata",
-            ),
-
-            # filetype/pyversion errors.
-            (
-                {
-                    "metadata_version": "1.2",
-                    "name": "example",
-                    "version": "1.0",
-                    "md5_digest": "bad",
-                },
-                "filetype: This field is required.",
-            ),
-            (
-                {
-                    "metadata_version": "1.2",
-                    "name": "example",
-                    "version": "1.0",
-                    "filetype": "bdist_wat",
-                },
-                "Error: Python version is required for binary distribution "
-                "uploads."
-            ),
-            (
-                {
-                    "metadata_version": "1.2",
-                    "name": "example",
-                    "version": "1.0",
-                    "filetype": "bdist_wat",
-                    "pyversion": "1.0",
-                    "md5_digest": "bad",
-                },
-                "filetype: Unknown type of file.",
-            ),
-            (
-                {
-                    "metadata_version": "1.2",
-                    "name": "example",
-                    "version": "1.0",
-                    "filetype": "sdist",
-                    "pyversion": "1.0",
-                },
-                "Error: The only valid Python version for a sdist is "
-                "'source'."
-            ),
-
-            # digest errors.
-            (
-                {
-                    "metadata_version": "1.2",
-                    "name": "example",
-                    "version": "1.0",
-                    "filetype": "sdist",
-                },
-                "Error: Must include at least one message digest."
-            ),
-            (
-                {
-                    "metadata_version": "1.2",
-                    "name": "example",
-                    "version": "1.0",
-                    "filetype": "sdist",
-                    "sha256_digest": "an invalid sha256 digest",
-                },
-                "sha256_digest: "
-                "Must be a valid, hex encoded, SHA256 message digest."
-            ),
-
-            # summary errors
-            (
-                {
-                    "metadata_version": "1.2",
-                    "name": "example",
-                    "version": "1.0",
-                    "filetype": "sdist",
-                    "md5_digest": "a fake md5 digest",
-                    "summary": "A" * 513,
-                },
-                "'" + "A" * 513 + "' is an invalid value for Summary. "
-                "Error: Field cannot be longer than 512 characters. "
-                "see "
-                "https://packaging.python.org/specifications/core-metadata",
-            ),
-            (
-                {
-                    "metadata_version": "1.2",
-                    "name": "example",
-                    "version": "1.0",
-                    "filetype": "sdist",
-                    "md5_digest": "a fake md5 digest",
-                    "summary": "A\nB",
-                },
-                ("{!r} is an invalid value for Summary. ".format('A\nB') +
-                 "Error: Multiple lines are not allowed. "
-                 "see "
-                 "https://packaging.python.org/specifications/core-metadata"),
-            ),
-
-            # classifiers are a FieldStorage
-            (
-                {
-                    "metadata_version": "1.2",
-                    "name": "example",
-                    "version": "1.0",
-                    "filetype": "sdist",
-                    "classifiers": FieldStorage(),
-                },
-                "classifiers: Should not be a tuple.",
-            ),
-
-            # keywords are a FieldStorage
-            (
-                {
-                    "metadata_version": "1.2",
-                    "name": "example",
-                    "version": "1.0",
-                    "filetype": "sdist",
-                    "keywords": FieldStorage(),
-                },
-                "keywords: Should not be a tuple.",
-            ),
-        ],
+        [({}, "None is an invalid value for Metadata-Version. " "Error: This field is required. " "see " "https://packaging.python.org/specifications/core-metadata"), ({"metadata_version": "-1"}, "'-1' is an invalid value for Metadata-Version. " "Error: Unknown Metadata Version " "see " "https://packaging.python.org/specifications/core-metadata"), ({"metadata_version": "1.2"}, "'' is an invalid value for Name. " "Error: This field is required. " "see " "https://packaging.python.org/specifications/core-metadata"), ({"metadata_version": "1.2", "name": "foo-"}, "'foo-' is an invalid value for Name. " "Error: Must start and end with a letter or numeral and " "contain only ascii numeric and '.', '_' and '-'. " "see " "https://packaging.python.org/specifications/core-metadata"), ({"metadata_version": "1.2", "name": "example"}, "'' is an invalid value for Version. " "Error: This field is required. " "see " "https://packaging.python.org/specifications/core-metadata"), ({"metadata_version": "1.2", "name": "example", "version": "dog"}, "'dog' is an invalid value for Version. " "Error: Must start and end with a letter or numeral and " "contain only ascii numeric and '.', '_' and '-'. " "see " "https://packaging.python.org/specifications/core-metadata"), ({"metadata_version": "1.2", "name": "example", "version": "1.0", "md5_digest": "bad"}, "filetype: This field is required."), ({"metadata_version": "1.2", "name": "example", "version": "1.0", "filetype": "bdist_wat"}, "Error: Python version is required for binary distribution " "uploads."), ({"metadata_version": "1.2", "name": "example", "version": "1.0", "filetype": "bdist_wat", "pyversion": "1.0", "md5_digest": "bad"}, "filetype: Unknown type of file."), ({"metadata_version": "1.2", "name": "example", "version": "1.0", "filetype": "sdist", "pyversion": "1.0"}, "Error: The only valid Python version for a sdist is " "'source'."), ({"metadata_version": "1.2", "name": "example", "version": "1.0", "filetype": "sdist"}, "Error: Must include at least one message digest."), ({"metadata_version": "1.2", "name": "example", "version": "1.0", "filetype": "sdist", "sha256_digest": "an invalid sha256 digest"}, "sha256_digest: " "Must be a valid, hex encoded, SHA256 message digest."), ({"metadata_version": "1.2", "name": "example", "version": "1.0", "filetype": "sdist", "md5_digest": "a fake md5 digest", "summary": "A" * 513}, "'" + "A" * 513 + "' is an invalid value for Summary. " "Error: Field cannot be longer than 512 characters. " "see " "https://packaging.python.org/specifications/core-metadata"), ({"metadata_version": "1.2", "name": "example", "version": "1.0", "filetype": "sdist", "md5_digest": "a fake md5 digest", "summary": "A\nB"}, ("{!r} is an invalid value for Summary. ".format('A\nB') + "Error: Multiple lines are not allowed. " "see " "https://packaging.python.org/specifications/core-metadata")), ({"metadata_version": "1.2", "name": "example", "version": "1.0", "filetype": "sdist", "classifiers": FieldStorage()}, "classifiers: Should not be a tuple."), ({"metadata_version": "1.2", "name": "example", "version": "1.0", "filetype": "sdist", "keywords": FieldStorage()}, "keywords: Should not be a tuple.")],
+        # metadata_version errors.
+        # name errors.
+        # version errors.
+        # filetype/pyversion errors.
+        # digest errors.
+        # summary errors
+        # classifiers are a FieldStorage
+        # keywords are a FieldStorage
     )
-    def test_fails_invalid_post_data(self, pyramid_config, db_request,
-                                     post_data, message):
+    def test_fails_invalid_post_data(
+        self, pyramid_config, db_request, post_data, message
+    ):
         pyramid_config.testing_securitypolicy(userid=1)
         db_request.POST = MultiDict(post_data)

@ambv ambv changed the title black seems to freak out when encountering comments inside a function call Standalone comments inside bracketed expressions sometimes block the expression from wrapping Mar 23, 2018
@ambv ambv changed the title Standalone comments inside bracketed expressions sometimes block the expression from wrapping Standalone comments inside nested bracketed expressions block the expression from wrapping Mar 29, 2018
@wbolster
Copy link
Contributor

Another example of this with long SQLAlchemy queries containing some inline comments in long chained calls like x.filter().order_by().all():

def foo(list_a, list_b):
    results = (
        User.query
        .filter(User.foo == 'bar')  # Because foo.
        .filter(db.or_(
            User.field_a.astext.in_(list_a),
            User.field_b.astext.in_(list_b),
        ))
        .filter(User.xyz.is_(None))
        # Another comment about the filtering on is_quux goes here.
        .filter(db.not_(
            User.is_pending.astext.cast(db.Boolean).is_(True)))
        .order_by(User.created_at.desc())
        .with_for_update(key_share=True)
        .all())
    return results

becomes

def foo(list_a, list_b):
    results = (
        User.query.filter(User.foo == 'bar').filter(db.or_(User.field_a.astext.in_(list_a), User.field_b.astext.in_(list_b))).filter(User.xyz.is_(None)).filter(db.not_(User.is_pending.astext.cast(db.Boolean).is_(True))).order_by(User.created_at.desc()).with_for_update(key_share=True).all()  # Because foo.
        # Another comment about the filtering on is_quux goes here.
    )
    return results

This produces extremely long lines, and moves the comments around to the wrong places.

@ambv ambv closed this as completed in c55d08d Mar 30, 2018
@ambv
Copy link
Collaborator

ambv commented Mar 30, 2018

@wbolster, Black will not support the "fluent interfaces" indentation style that you're employing in your SQLAlchemy queries. Your example is reformatted like this:

def foo(list_a, list_b):
    results = (
        User.query.filter(User.foo == 'bar').filter(  # Because foo.
            db.or_(User.field_a.astext.in_(list_a), User.field_b.astext.in_(list_b))
        ).filter(
            User.xyz.is_(None)
        )
        # Another comment about the filtering on is_quux goes here.
        .filter(db.not_(User.is_pending.astext.cast(db.Boolean).is_(True))).order_by(
            User.created_at.desc()
        ).with_for_update(
            key_share=True
        ).all()
    )
    return results

See #67 for more reference.

ambv added a commit that referenced this issue Mar 30, 2018
@dstufft
Copy link

dstufft commented Mar 30, 2018

Damn, I find the fluent interfaces way nicer for SQLAlchemy queries :) ah well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F: comments The syntactic kind. Not in the language grammar, always on our minds. Best bugs. T: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants