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

pyupgrade of beets to Python 3.6 #4030

Merged
merged 27 commits into from
Sep 28, 2021
Merged

pyupgrade of beets to Python 3.6 #4030

merged 27 commits into from
Sep 28, 2021

Conversation

arogl
Copy link
Contributor

@arogl arogl commented Aug 26, 2021

WIP - Attempt to upgrade to Python 3.6 as a minimum

All tests pass/fail the same as Master in both Windows (PY38) and Linux (PY38)

Pushing to get the tests to run

I used the following tool to convert https://github.com/asottile/pyupgrade
pyupgrade --py36-plus

To Do

  • Documentation. (If you've add a new command-line flag, for example, find the appropriate page under docs/ to describe it.)
  • Changelog. (Add an entry to docs/changelog.rst near the top of the document.)
  • Tests. (Encouraged but not strictly required.)

@arogl
Copy link
Contributor Author

arogl commented Aug 26, 2021

I do not know why py-lint fails, but running the same command flake8 beets beetsplug beet test setup.py docs works

@arogl
Copy link
Contributor Author

arogl commented Aug 26, 2021

I found the following for the Python 3.10 failures:

sphinx-doc/sphinx#9513

@sampsyo
Copy link
Member

sampsyo commented Aug 26, 2021

Cool; many thanks for wrangling this!! A detailed line-by-line review is also in order, but this all looks pretty good on a (very) cursory glance.

As for the lint error, I think we'll want to remove flake8-future-import altogether, including its requirement in setup.py:

beets/setup.py

Line 124 in 75223ee

'flake8-future-import',

And its configuration in setup.cfg:

# future-import errors

It's likely that you're not seeing the errors locally because flake8-future-import (or some other plugin) is not installed?

@arogl
Copy link
Contributor Author

arogl commented Aug 26, 2021

That was it 👍

We also need to remove the flake8-coding plugin

@sampsyo
Copy link
Member

sampsyo commented Aug 26, 2021

Great; good catch! I had to look it up, but the default source encoding was indeed UTF-8 as of 3.0, thanks to PEP 3120.

@arogl
Copy link
Contributor Author

arogl commented Aug 26, 2021

sys.version tests are next

@arogl arogl mentioned this pull request Aug 27, 2021
3 tasks
@arogl
Copy link
Contributor Author

arogl commented Sep 24, 2021

How do you want me to pull in the updates to Master?
per commit or by Pull rrquest

@sampsyo
Copy link
Member

sampsyo commented Sep 25, 2021

Ah, great question—sorry for letting this fall out of date! Any option is fine with me, TBH (merging in master here, rebasing, etc.)—all options seem perfectly good to me.

Copy link
Member

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow! This is an enormous diff, but I think I've now reviewed the whole thing. I only found a few very minor/superficial comments. Aside from that, I think we should merge this as soon as possible! So excited to be rid of the dependency on six.

@@ -108,7 +106,7 @@ def assign_items(items, tracks):
log.debug('...done.')

# Produce the output matching.
mapping = dict((items[i], tracks[j]) for (i, j) in matching)
mapping = {items[i]: tracks[j] for (i, j) in matching}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a clever improvement. Nice work, pyupgrade.

BASE_URL = 'https://musicbrainz.org/'
else:
BASE_URL = 'http://musicbrainz.org/'
BASE_URL = 'https://musicbrainz.org/'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really great that we can get rid of this check.

values = dict((k, v) for (k, v) in cols.items()
if not k[:4] == 'flex')
values = {k: v for (k, v) in cols.items()
if not k[:4] == 'flex'}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be k.endswith('flex'), I suppose.

@@ -1317,17 +1317,16 @@ def read_tasks(session):

# Generate tasks.
task_factory = ImportTaskFactory(toppath, session)
for t in task_factory.tasks():
yield t
yield from task_factory.tasks()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wahoo! One of my favorite Python 3 quality-of-life syntax features.

'album_id': types.FOREIGN_ID,

'title': types.STRING,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little sad to lose this table-like formatting, which did make this listing more readable, but I suppose it's not that big of a deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to reformat

else:
# On Python 3, python-mpd2 always uses Unicode
self.client = mpd.MPDClient()
# On Python 3, python-mpd2 always uses Unicode
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is maybe not necessary anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleted

@@ -1,5 +1,5 @@
[flake8]
min-version = 2.7
min-version = 3.6
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch; I left this one out.

@@ -87,7 +85,6 @@ def build_manpages():
},

install_requires=[
'six>=1.9',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 🎉 🎉

Comment on lines 28 to 30
# if platform.system() == 'Windows' and PY2:
# in_file = in_file.decode('utf-8')
# out_file = out_file.decode('utf-8')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably OK to delete?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deleted

@arogl
Copy link
Contributor Author

arogl commented Sep 28, 2021

@sampsyo I believe I have addressed the feedback

@sampsyo
Copy link
Member

sampsyo commented Sep 28, 2021

Truly heroic, @arogl. This is so awesome.

@sampsyo sampsyo merged commit 018396c into beetbox:master Sep 28, 2021
sampsyo added a commit that referenced this pull request Sep 28, 2021
@arogl arogl deleted the pyupgrade branch May 5, 2023 06:19
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.

2 participants