search

jupyterhub / batchspawner

Custom Spawner for Jupyterhub to start servers in batch scheduled systems
BSD 3-Clause "New" or "Revised" License
190 stars 134 forks source link

General maintenance: autoformat, flake8, readme badges, misc in github workflows #208

Closed consideRatio closed 3 years ago

consideRatio commented 3 years ago

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.

welcome[bot] commented 3 years ago

Thanks for submitting your first pull request! You are awesome! :hugs:
If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly. welcome You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! :wave:
Welcome to the Jupyter community! :tada:

consideRatio commented 3 years ago

@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.

rkdarst commented 3 years ago

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 . [2] Another was explaining how to format it locally. I think directing to JH docs is plenty for that, we can handle it later.

rkdarst commented 3 years ago

@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.

consideRatio commented 3 years ago

@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.

rcthomas commented 3 years ago

@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!

mbmilligan commented 3 years ago

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.

consideRatio commented 3 years ago

@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 https://github.com/jupyterhub/batchspawner/pull/208#discussion_r615178086.

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).

mbmilligan commented 3 years ago

Thank you for updating the branch. This looks acceptable to me now.

welcome[bot] commented 3 years ago

Congrats on your first merged pull request in this project! :tada: congrats Thank you for contributing, we are very proud of you! :heart:

consideRatio commented 3 years ago

Thanks @mbmilligan!