don't require typed-ast

[KotlinIsland]

#2044

[cooperlees]

Thanks! Looks good to me and what I’d had planned to do.

Can you please just add a line to CHANGES.md please. I’ll try get some CI added to nudge for this today.

[cooperlees]

O my bad, I missed CI for >=3.8 is broken. Let’s look into that.

[ichard26]

I have some serious concerns about the implementation here and the general side effects of such a change. A review with more detail will come eventually.

[KotlinIsland]

If python2 is optional now, does that mean test configuration should be different? ie: don't run python2 tests under normal configuration, run python2 tests under the optional 'python2' configuration?

[KotlinIsland]

What on earth is this error, it looks like absolute nonsense.

> pip install -e .[d] .[python2]

...

INFO: pip is looking at multiple versions of black[d] to determine which version is compatible with other requirements. This could take a while.
ERROR: Cannot install black[d]==20.8b2.dev112+gde5d07a.d20210321 and black[python2]==20.8b2.dev112+gde5d07a.d20210321 because these package versions have c
onflicting dependencies.

The conflict is caused by:
    black[d] 20.8b2.dev112+gde5d07a.d20210321 depends on black 20.8b2.dev112+gde5d07a.d20210321 (from C:\Users\Morgan\IdeaProjects\black)
    black[python2] 20.8b2.dev112+gde5d07a.d20210321 depends on black 20.8b2.dev112+gde5d07a.d20210321 (from C:\Users\Morgan\IdeaProjects\black)

To fix this you could try to:
1. loosen the range of package versions you've specified
2. remove package versions to allow pip attempt to solve the dependency conflict

Removing -e from the command fixed it.

[KotlinIsland]

test_piping failed because of black.assert_stable(source, result.output, DEFAULT_MODE) which has experimental_string_processing enabled.

Mode(target_versions=set(), line_length=88, string_normalization=True, magic_trailing_comma=True, experimental_string_processing=True, is_pyi=False)
--- first pass
+++ second pass
@@ -6349,12 +6349,13 @@
                 return ast3.parse(src, filename, feature_version=feature_version)
             except SyntaxError:
                 continue
     if ast27.__name__ == "ast":
         print(
-            "Black attempted to parse python2 code but the typed_ast package is not installed.\n"
-            "You must install typed_ast with `python3 -m pip install typed-ast`.",
+            "Black attempted to parse python2 code but the typed_ast package is not"
+            " installed.\nYou must install typed_ast with `python3 -m pip install"
+            " typed-ast`.",
             file=sys.stderr,
         )
         sys.exit(1)
     return ast27.parse(src)

[KotlinIsland]

All passed 🎉

Currently tests are run with black[python2] I think additional configurations should be added for python 3.8 with black and not black[python2] where python2 tests are not run.

[KotlinIsland]

Wait a second, why is the dependency on typed-ast here in the first place? is it just to parse python 2? if so the dependencies should not be "typed-ast>=1.4.2; python_version < '3.8'",, the dependency should instead be removed entirely.

[KotlinIsland]

I noticed that ast.parse doesn't include type comments by default.

If type_comments=True is given, the parser is modified to check and return type comments
-https://docs.python.org/3/library/ast.html#ast.parse

Should the code be updated to include this parameter?

    if sys.version_info >= (3, 8):
        # TODO: support Python 4+ ;)
        for minor_version in range(sys.version_info[1], 4, -1):
            try:
                return ast.parse(
                    src,
                    filename,
                    feature_version=(3, minor_version),
                    type_comments=True,
                )
            except SyntaxError:
                continue

[KotlinIsland]

@cooperlees Could you review/address this please.

[ichard26]

Hey thanks for the PR! I know the rest of my review is quite negative, but I genuinely appreciate the time and energy you've put into putting this together!

I don't like this change because it provides the wrong feedback to a not an insignificant group of users. If I (as a Python 3 only user) give Black an invalid file, the Python 3.X parse attempts will fail and then Black exits with a "Black attempted to parse python2 code, [...] please install typed_ast" error... which makes no sense to me as a Python 3 only user. Not only is this hiding the true problem: the input file isn't valid Python 3 code, the confused user, might try the advice to install typed_ast which 1) might not work or be a pain, and 2) doesn't actually the fix the issue.

Personally I'm open to pushing most if not all of the disruption to the users still using Black's Python 2 support. My idea would be that Black would need --target-version py27 (configured automatically via magic or configured manually) to even attempt Python 2 parsing. Although this falls apart when Black is formatting multiple files at once (since someone is bound to have a mix of Python 2 and 3 files... of which our target-version logic can't really describe).

There's not much actionable in these two paragraphs of feedback. It's something to just think about, perhaps someone else (maybe you!) has a better idea!

[ichard26]

I noticed that ast.parse doesn't include type comments by default.

If type_comments=True is given, the parser is modified to check and return type comments
-https://docs.python.org/3/library/ast.html#ast.parse

Should the code be updated to include this parameter?

I mean, maybe? It may increase safety but let's do that in another PR since it might have an effect we haven't foreshadowed.

[ichard26]

There's not much actionable in these two paragraphs of feedback. It's something to just think about, perhaps someone else (maybe you!) has a better idea!

Actually thinking about this more, we could make parse_ast use the parsing attempt information from the first parse (IRCC lib2to3_parse) to configure whether Python 2 parsing should even be attempted. This would also fix the long-standing issue where our forked AST+CST blib2to3 parser (for formatting) uses a different grammar than the built-in or third-party typed_ast parser (to run safety checks) causing misleading and confusing errors. see also #1038 (comment)

[KotlinIsland]

@ichard26 Thanks for the review! Could you please address #2053 (comment)

[ichard26]

Wait a second, why is the dependency on typed-ast here in the first place? is it just to parse python 2? if so the dependencies should not be "typed-ast>=1.4.2; python_version < '3.8'",, the dependency should instead be removed entirely.

Probably something to do with type comments but I'm not sure.

[KotlinIsland]

Probably something to do with type comments but I'm not sure.

This seems to be the only usage of type comments. But this won't work with python >= 3.8 because we aren't using the type_comments=True option
__init__.py line 6389

        type_ignore_classes = (ast3.TypeIgnore, ast27.TypeIgnore)
        if sys.version_info >= (3, 8):
            type_ignore_classes += (ast.TypeIgnore,)

[KotlinIsland]

Only remaining issue in my opinion is that all tests use python2 option. I have no idea how to go about setting up a different test configuration and only executing certain tests.

[cooperlees]

Only remaining issue in my opinion is that all tests use python2 option. I have no idea how to go about setting up a different test configuration and only executing certain tests.

I'd love @ambv be the definitive answer here. But I feel we can move the python2 tests to a separate GitHub action with the plan to eventually deprecate and no longer support. If we were already "stable" I would have said we freeze python2 supported at version X. But we're not there.

Others thoughts?

[ambv]

I approve this in general. Good work. There's two nits before we merge this.

Ideally, tests should be running twice: with the python2 extra and without it. We can do this in a separate pull request.

[ambv]

Tests with the python2 extra can be a separate environment in tox.ini, just like fuzz tests are. We need to make sure they are ran on CI.

[KotlinIsland]

Tests with the python2 extra can be a separate environment in tox.ini, just like fuzz tests are. We need to make sure they are ran on CI.

I assuming it needs different configuration to not run python 2 tests, And a negative test that expects SyntaxError with python2.

[ulgens]

@KotlinIsland @ambv I'm using Black (commit 201b331 - latest from master) with Python 3.9. It still requires typed-ast package and fails with

from typed_ast import ast3, ast27
ModuleNotFoundError: No module named 'typed_ast'

[KotlinIsland]

😳
Oops ModuleNotFoundError only catching ImportError

[ichard26]

Isn't ModuleNotFoundError supposed to be a subclass of ImportError and therefore should be caught in that try except block?

https://docs.python.org/3.9/library/exceptions.html#exception-hierarchy