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

Execution fixes plus black #8

Merged
merged 5 commits into from
Sep 19, 2018
Merged

Execution fixes plus black #8

merged 5 commits into from
Sep 19, 2018

Conversation

syrusakbary
Copy link
Member

This PR addresses the comments from #6.

In summary:

  • Fixed execute function (I modified the PR inline using Github editor and I missed some things)
  • Added black autoformatting (including Trevis tests)

@Cito
Copy link
Member

Cito commented Sep 18, 2018

The switch to black (doublequotes) broke the regular expression in setup.py.

Also, even though the maximum line length has been increased from 79 to 88, we now get "line too long" errors due to the automatic indentation (in my manual styling I spared levels where it made sense). So we need to add some # noqa comments to ingore these errors.

@Cito
Copy link
Member

Cito commented Sep 18, 2018

I noticed that black reformats code like this:

f(g(h(
    """
    Some text
    """)))

into that:

f(
    g(
        h(
            """
    Some text
    """
        )
    )
)

This increases both line number and line lengths and I don't think it's better readable.

I really don't like that. Also, this begs for the multiline-string to be manually adapted to the new identation level, like this:

f(
    g(
        h(
            """
            Some text
            """
        )
    )
)

What do you think? Should we adapt such formatting?

But that would mean that many tests that check column positions would need to be adapted as well and line length becomes even longer.

See also psf/black#500 which is a similar issue and leads to the problem of long line lengths mentioned above.

@Cito
Copy link
Member

Cito commented Sep 18, 2018

Another thing to consider: I hand-crafted the docstrings to fit into the 79 char line limit. Does this mean we should now allow 88 chars in docstrings? Or should we continue to restrict docstrings to 79 chars? But I would hate to have two different standards for line lengths depending on whether it's code or docs. Editors usually only support one global setting for the max line lenght (wrap and visual guide).

@syrusakbary
Copy link
Member Author

syrusakbary commented Sep 18, 2018

What do you think? Should we adapt such formatting?

I think you are right, as the conversion you showcased is not ideal. But looking at the code reformatted (primarily on the tests dir, which is the most affected by this), I think the readability is quite close to the previous non-black formatted version.
If you want, we can take tests dir out of black reformatting, and just run flake8 on it. Thoughts?

Another thing to consider: I hand-crafted the docstrings to fit into the 79 char line limit. Does this mean we should now allow 88 chars in docstrings? Or should we continue to restrict docstrings to 79 chars?

I'm not sure about this one, I think that as long doesn't go over 88 chars it should be fine, so we can move slowly the docstrings to 88 chars in the long run. Personally I think this is more a stylistic question, so I will go with whatever you prefer here :)

@Cito
Copy link
Member

Cito commented Sep 18, 2018

If you want, we can take tests dir out of black reformatting, and just run flake8 on it. Thoughts?

I think we should be consequent and use the same formatting everywhere. Consistency inside a project is very important. Also, many IDEs and editors only support global settings for the formatting tool and line length. That's another reason to use the 88 char line limit also for docstrings and docs (text files) as well.

I'm still a unhappy with the double quotes and forced new lines and indentations for parens. Also, black is not directly supported by my favorite editor PyCharm. But I guess I need to swallow that black pill even if it's not quite my taste. :)

@Cito
Copy link
Member

Cito commented Sep 18, 2018

But actually I was also requesting your feedback regarding whether we should adjust the indentation inside multi-line strings to make them fit the indentation of the opening triple-quote (see the last two code snippets above, both of which are black-compliant, i.e. will not be reformatted by black). Black cannot change the indentation inside multi-line strings, because that changes semantics. But we could do that (if we make the corresponding adaptations to checks for column positions). The disadvantage is that the available line length may become very short in some cases with large indentation. Maybe indent properly when the strings are not so long and reduce indentation to make lines fit 88 chars otherwise?

@Cito Cito merged commit 1bcd466 into master Sep 19, 2018
@Cito
Copy link
Member

Cito commented Sep 19, 2018

Maybe I'm overthinking this. Some of the code looks uglier to me now, but it's not a big deal. Will reformat the docstrings over time. I think we can close this and move on.

@syrusakbary
Copy link
Member Author

@Cito thanks for your flexibility on the code formatting. I hope bringing uniformity within all GraphQL Python packages will benefit the community.

PS: Just also seen your commit formatting the docs towards 88-char length :)

@Cito Cito deleted the execution-fixes-plus-black branch October 22, 2018 19:49
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

2 participants