Use black and isort for py files.

PIG208 commented 3 years ago

We add black and isort using the configuration from Zulip and reformat the project.

https://chat.zulip.org/#narrow/stream/127-integrations/topic/black.20and.20isort

andersk commented 3 years ago

One of the manual pre-fixes we needed to do in the server was for multi-line string literal concatenations that Black moved to the same line. That is the case here too, e.g.:

--- a/zulip_bots/zulip_bots/bots/dropbox_share/dropbox_share.py
+++ b/zulip_bots/zulip_bots/bots/dropbox_share/dropbox_share.py
@@ -168,11 +187,11 @@ def dbx_write(client: Any, fn: str, content: str) -> str:
         result = client.files_upload(content.encode(), fn)
         msg = "Written to file: " + URL.format(name=result.name, path=result.path_lower)
     except Exception:
-        msg = "Incorrect file path or file already exists.\n"\
-              "Usage: `write <filename> CONTENT`"
+        msg = "Incorrect file path or file already exists.\n" "Usage: `write <filename> CONTENT`"

     return msg

Easiest fix is to write this as a single string literal:

        msg = "Incorrect file path or file already exists.\nUsage: `write <filename> CONTENT`"

You can find these all with

$ pylint -d all -e implicit-str-concat $(git ls-files '*.py'; git grep -Pl '\A#!.*\bpython' -- . ':!*.py')
************* Module zulip_bots.bots.dropbox_share.dropbox_share
zulip_bots/zulip_bots/bots/dropbox_share/dropbox_share.py:190:0: W1404: Implicit string concatenation found in assignment (implicit-str-concat)
zulip_bots/zulip_bots/bots/dropbox_share/dropbox_share.py:250:0: W1404: Implicit string concatenation found in assignment (implicit-str-concat)
************* Module zulip_bots.bots.dropbox_share.test_dropbox_share
zulip_bots/zulip_bots/bots/dropbox_share/test_dropbox_share.py:166:0: W1404: Implicit string concatenation found in assignment (implicit-str-concat)
zulip_bots/zulip_bots/bots/dropbox_share/test_dropbox_share.py:230:0: W1404: Implicit string concatenation found in assignment (implicit-str-concat)
************* Module zulip_bots.bots.game_handler_bot.game_handler_bot
zulip_bots/zulip_bots/bots/game_handler_bot/game_handler_bot.py:50:0: W1404: Implicit string concatenation found in assignment (implicit-str-concat)
************* Module zulip_bots.bots.game_of_fifteen.game_of_fifteen
zulip_bots/zulip_bots/bots/game_of_fifteen/game_of_fifteen.py:131:0: W1404: Implicit string concatenation found in assignment (implicit-str-concat)
************* Module libraries.game
zulip_bots/zulip_bots/bots/merels/libraries/game.py:45:0: W1404: Implicit string concatenation found in assignment (implicit-str-concat)
************* Module zulip_bots.bots.twitpost.test_twitpost
zulip_bots/zulip_bots/bots/twitpost/test_twitpost.py:44:0: W1404: Implicit string concatenation found in assignment (implicit-str-concat)
************* Module zulip_bots.bots.yoda.yoda
zulip_bots/zulip_bots/bots/yoda/yoda.py:112:0: W1404: Implicit string concatenation found in assignment (implicit-str-concat)

timabbott commented 3 years ago

I reordered commits and merged the first 4 (which add configuration but don't run lint yet) as the series ending with 4083849b5dc4ce6ab78f3f3a4ff01ae6d497b6c7. This should make it easier to script moving a PR across the transition.

A few notes on things that are a big ugly while skimming the changes, basically all things we had with the server project too:

We have various strings using % formatting that could be switched to f-strings and that would prevent messy line-wrapping. @andersk did you have a tool to bulk-migrate to f-strings we should run?
Some of our argparse usage could line-wrap less if we specified fewer optional parameters. I don't think it's worth delaying to try to do something about that.
Some of the docstrings for the main API bindings project end up a bit weird, but not clearly worse than before. Ultimately we should think about whether it makes sense to just drop those entirely and instead just have the docstrings link to the API documentation for the relevant endpoint, since I suspect otherwise they'll just end up out-of-date with time.
Some of the examples, like zulip/zulip/examples/create-user, end up with somewhat messy indentation; it could be worth a pre-fix on those to do result = client.client_user(...)\nprint(result) for better readability. I think that's probably worth doing just because these are intended to be copyable example code.

Overall I'm fine with merging this and fixing forward, since the things where a pre-fix could be helpful feel pretty rare. @andersk I assume you'll do the merge + announcement for how to rebase PRs when you're happy with the pre-fixing story, since you generated those instructions last time.

PIG208 commented 3 years ago

I tried to use flynt to convert all string literals to use the f-string format. Overall, it looks pretty good. But I'm reluctant to actually add it as a linter since the result it produces might still need to be reformatted by black. I think it is a good tool for one-time migration and we can enforce the use of f-strings for the future PRs.

eeshangarg commented 3 years ago

@PIG208 Thanks for working on this! This PR looks pretty good overall! Before reviewing the whole PR, just skimming it made me want to mention a couple of things:

This is a huge PR! Would perhaps merging this commit by commit be better?
@timabbott, @PIG208 I notice that migrating to f-style string formatting is really hard to do in bulk in all cases. Overall, it looks pretty good, but we clearly missed some edge cases in this PR that need a manual review. Maybe we should take a more modular approach and split out string migration into a separate PR?
As for API examples ending up with messy indentation, this again makes me think that a lot of the formatting done by Black should maybe be split up into distinct chunks/PRs. As an example, we could do this on a per-PyPI-package basis, where we split out the bulk formatting for the zulip-botserver, zulip, and zulip_bots packages.

The only concern I have with merging this as-is is that it makes it harder to go back and take a deeper look at some of the edge cases that require manual intervention and were missed by the automated migration of string formatting styles. A more modular set of PRs would allow us to avoid having files that use f-style strings but are also riddled with calls to .format, as is the case currently.

Let me know what you all think, thanks! :)

PIG208 commented 3 years ago

@timabbott, @PIG208 I notice that migrating to f-style string formatting is really hard to do in bulk in all cases. Overall, it looks pretty good, but we clearly missed some edge cases in this PR that need a manual review. Maybe we should take a more modular approach and split out string migration into a separate PR?

I guess we can split the pyupgrade related things to another PR and deprioritize them at the moment. Maybe it will be much better (and much easier) for us to work on that if we can get the black and isort related changes merged first.

As for API examples ending up with messy indentation, this again makes me think that a lot of the formatting done by Black should maybe be split up into distinct chunks/PRs. As an example, we could do this on a per-PyPI-package basis, where we split out the bulk formatting for the zulip-botserver, zulip, and zulip_bots packages.

Yes, I think this might be a good direction.

eeshangarg commented 3 years ago

@PIG208 Yes, I agree with getting the bulk of the black and isort changes merged. After that, we can inspect the string formatting migration changes more closely. A quick check I would do is to do a git grep for ".format(" on top of your pyupgrade commits to see if there are any leftover strings that weren't migrated, my guess is you will discover a few that flynt didn't catch. Thanks! :)

timabbott commented 3 years ago

I'm not sure about backing out the pyupgrade changes; let me take a look and see if I'd be happier just merging this as this PR plans. Also, I don't want to split the Black migration to multiple PRs, because that'll make instructions for rebasing harder.

timabbott commented 3 years ago

Yeah, after reviewing this, I think merging this as-is makes sense. Huge thanks for all the work on this @PIG208!

Can you post a chat.zulip.org comment similar to https://chat.zulip.org/#narrow/stream/2-general/topic/black/near/1117538 for how to rebase across the isort+Black changes without manually resolving merge conflicts?

I think we should open a few follow-up workstreams:

Going through the open PRs on this project, which are essentially all super stale, and trying to work out which are useful (and worth rebasing/finishing) and which should be closed.
Discussing whether we should just delete the example docstrings in this project because they duplicate the official API documentation.

PIG208 commented 3 years ago

Can you post a chat.zulip.org comment similar to https://chat.zulip.org/#narrow/stream/2-general/topic/black/near/1117538 for how to rebase across the isort+Black changes without manually resolving merge conflicts?

I have posted the instruction on chat.zulip.org. However, probably because some of the manual fixes aren't separated into different commits, the script I wrote cannot ideally eliminate all the diffs brought by this PR.

timabbott commented 3 years ago

Yeah, I think that's OK -- we don't have many huge open PRs, so most of them are likely to be fairly contained to a small number of files that don't require much manual fixups.

zulip / python-zulip-api

Use black and isort for py files. #692