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

pre-commit #95

Merged
merged 2 commits into from
Mar 9, 2021
Merged

pre-commit #95

merged 2 commits into from
Mar 9, 2021

Conversation

ocefpaf
Copy link
Collaborator

@ocefpaf ocefpaf commented Mar 9, 2021

@efiring this is one of those PRs that you don't like 😄 Lots of churn!

If you have local changes and this will complicate stuff feel free to just close it. If not, I recommend merging this so next time pre-commit run we will keep the code tidy, it could help with a future refactor.

@ocefpaf ocefpaf requested a review from efiring March 9, 2021 19:16
@efiring
Copy link
Collaborator

efiring commented Mar 9, 2021

Looks like it is blacker than black! I see you added two code fixers. Some of the extra commas surprised me, but I guess the advantage is that they signal that the block is not yet closed; there is another bracket or paren on the next line. I don't actually like that business of putting the closing paren on its own line and out-dented--it seems a bit unpythonic, looks more like C formatting--but I can tolerate it. That might be the only black rule that I really dislike.
I like the switch to f-strings and direct dictionary comprehensions without having to do it manually.
My local changes are tentative and not extensive at the moment, so I will merge. I don't think it will be too disruptive.

@efiring efiring merged commit 5f15bcb into wesleybowman:master Mar 9, 2021
@ocefpaf ocefpaf deleted the pre-commit branch March 9, 2021 22:05
@ocefpaf
Copy link
Collaborator Author

ocefpaf commented Mar 9, 2021

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