General maintenance: autoformat, flake8, readme badges, misc in github workflows #208
Conversation
This comment has been minimized.
This comment has been minimized.
consideRatio
changed the title
consideRatio
force-pushed
the
pr/general-maintenance
branch
2 times, most recently
from
728fb57
to
83a6e8c
Compare
|
@rkdarst @mbmilligan @minrk this PR is going stale, could you help nudge this PR towards resolving? This repo is not the first to have the same kind of changes, but they have been adopted in various other repository within the jupyterhub org. |
|
We just talked about this in the monthly JupyterHub in HPC meeting. I think we will wait another month for the next meeting, when a few more key people are hopefully here. Development isn't too fast so risk of conflict isn't that great. But the general feeling is we think it's a good idea because standarizing is more important than our preferences. There's a bit of grumbling about auto-formatting[1][2], but the acceptance is we need to live with it. Thanks for working through all of this! [1] For me, the issue is mainly that code should be as readable as possible, and readability can be increased by conveying information in certain types of formatting. Auto-formatters can't understand this. I wrote up this long ago for someone who once auto-formatted all of my code, before black became a thing: http://rkd.zgib.net/wiki/Python/Style . |
|
@mbmilligan, if you agree, I guess all of us at the meeting yesterday would say 'let's go ahead and merge'. Anyone else, correct me if I'm wrong. |
|
@rkdarst thanks for considering this! I absolutely agree with the feeling it is a compromise on code readability when sections are made with intention for readability. The key value I find for autoformatting is that they reduce the complexity to review and track changes in PRs as the changes following that can be trusted to only be non-formatting related. This aspect has ended up being practically very relevant for several repo's across the org. |
|
@consideRatio @mbmilligan I think at the April HPC call we discussed that if this PR was brought up to date and conflicts resolved then it would be merged. Sound good? I think it's just 2 conflicts that need to be fixed, can you do that @consideRatio? Thanks! |
batchspawner/batchspawner.py
Outdated
| @@ -88,89 +88,98 @@ class BatchSpawnerBase(Spawner): | |||
| start_timeout = Integer(300).tag(config=True) | |||
|
|
|||
| # override default server ip since batch jobs normally running remotely | |||
| ip = Unicode("0.0.0.0", help="Address for singleuser server to listen at").tag(config=True) | |||
| ip = Unicode("0.0.0.0", help="Address for singleuser server to listen at").tag( | |||
There was a problem hiding this comment.
This is an abomination. If black is going to insist on breaking these lines, let's add trailing commas to the argument list so that it doesn't break the .tag(config=True) expression in a way that obscures the method chaining being used.
|
Yes, the team agreed that we should go ahead and adopt this workflow. I did a thorough review of the resulting changes and only found one really objectionable antipattern, an example of which is flagged in my comment above. It looks like that can be dealt with by adding some extra trailing commas to the trait definition parameter lists, and I'd like to see that done as part of this PR so we don't neglect it later. |
I've systematically added a trailing comma to traitlets where more than
one argument was passed and that had .tag(config=True) suffixed.
This was my understanding of a suggestion made by mbmilligan in this
commits associated PR.
Example:
```python
# before this commit (with black formatting)
ip = Unicode("0.0.0.0", help="Address for singleuser server to listen at").tag(
config=True
)
# after this commit (with black formatting)
ip = Unicode(
"0.0.0.0",
help="Address for singleuser server to listen at",
).tag(config=True)
# independent of this commit
batch_submit_cmd = Unicode("qsub").tag(config=True)
```
consideRatio
force-pushed
the
pr/general-maintenance
branch
from
83a6e8c
to
2848b59
Compare
consideRatio
force-pushed
the
pr/general-maintenance
branch
from
2848b59
to
c5fec6e
Compare
consideRatio
force-pushed
the
pr/general-maintenance
branch
from
6a0cb65
to
5dde989
Compare
|
@rcthomas and @mbmilligan thanks for following up on this! I have reapplied the changes on master and added commit 870eea0 with its comment on what I did to address #208 (comment). A test failure has shown up, but I could see it in other recent CI failures so it doesn't relate to this PR. I think the test failure is related to jupyterhub 0.9.6 (very old) not pinning its requirement on sqlalchemy hard enough given some more recent change to it (https://pypi.org/project/SQLAlchemy/#history). While investigating this CI failure I made the CI system not cancel all tests if a single test fails (706c560), and I also made the test run against jupyterhub 1.3.0 (5dde989). |
|
Thank you for updating the branch. This looks acceptable to me now. |
|
Congrats on your first merged pull request in this project! 🎉 |
|
Thanks @mbmilligan! |
General matinenance
Review notes
Only the autoformatting commits for black / prettier are large, but they are only that. Reviewing commit by commit is probably reasonable.