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

Application of PyUpgrade on the whole source #2753

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

heavelock
Copy link
Contributor

@heavelock heavelock commented Nov 19, 2020

I've seen that recently Apache Airflow ran pyupgrade on their codebase in order to get rid of the legacy code.

So I decided to give it a a go with running it on obspy. Here is the raw result of the running

pyupgrade --py37-plus `find obspy -name "*.py" -type f`

I did not dig any deeper than that, I wanted to consult first if that PR is something we want.

Changes are mostly concerning removal of % substitutions in strings, dict and set comprehensions, removal of encoding indicators or removal of inheritance of Object

Signed-off-by: Damian Kula <dkula@unistra.fr>
@megies
Copy link
Member

megies commented Nov 19, 2020

In principle, this is something we'd want to do, I'm all for it. Only skimmed parts of it but I especially like the newer super() calls. It's a shame it doesn't transition to pathlib as well, see #2751.

However, a PR with such a huge amount of changes has big potential to cause loads of merge conflicts and thus be really painful in terms of merging maintenance 1.2.x branch back into master. So this needs to be done as soon as a final 1.2.3 is released (be it full fledged with packages etc. or just as a pypi upload), and then merged right away so that newly opened PRs don't end up in rebase hell (potentially).

@megies megies marked this pull request as draft November 19, 2020 14:12
@d-chambers
Copy link
Member

Great idea! We should consider doing any other huge automatic refactoring at the same time, like applying the black formatter and/or auto converting docstrings to numpy style (if we still want to do those).

@megies
Copy link
Member

megies commented Nov 20, 2020

I'm not a huge fan of the styling that black enforces, actually, but if lots of people want to go with it.. well majority would win I guess. Numpy style docs would definitely not hurt, but it's a major effort, I guess, unless there's really good automatic converters..

@heavelock
Copy link
Contributor Author

I agree with megies about black - I am not a big fan of some of their choices. It's useful in terms of making it easier for new contributors but I am not sure how good reason is that. Also I prefer obspy's current docstring style but I guess it's just something I am used to - I much more often look into source of obspy rather than numpy. :P

@d-chambers
Copy link
Member

We certainly don't have to do either one of those if they aren't wanted, I was just making the point that it would be a good idea to do all the automatic changes that will touch a ton of files in one go.

@heavelock
Copy link
Contributor Author

heavelock commented Nov 23, 2020

That's true, if we do some massive automatic changes on the codebase it would be good if they were squashed into a single commit - in that case when going through some blames or logs, there is only a single commit to exclude.

@krischer
Copy link
Member

I'd quite strongly vote in favor of using black. I agree that the formatting in places is weird (although it can be configured quite a bit) but just having "format on save" is such a huge productivity boost and one really no longer even has to think about formatting because it just happens automatically in the background.

@d-chambers
Copy link
Member

d-chambers commented Nov 24, 2020

I share @krischer opinion. I didn't like some of black's choices at first (like how it wraps long functions to multiple lines) but I have since gotten use to it. Not having to think about formatting is a huge win.

@barsch
Copy link
Member

barsch commented Nov 25, 2020

+1 for black

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

5 participants