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

Improved NamedTuple declaration #5393

Merged
merged 10 commits into from
Aug 27, 2024
Merged

Improved NamedTuple declaration #5393

merged 10 commits into from
Aug 27, 2024

Conversation

amogus07
Copy link
Contributor

Description

Utilize a new way of declaring NamedTuples, which allows for typechecking as well. Maybe the latter is now redundant in other places, but I'm not that familiar with the codebase yet, so I just changed the declarations (and hopefully used the correct types). While I was at it, I also ran poetry update, but I'll revert poetry.lock in case I wasn't supposed to do that. This is my first commit here, so I hope I didn't do anything wrong...

To Do

  • Documentation.
  • Changelog.
  • Tests.

Copy link

Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry.

Copy link
Member

@snejus snejus left a comment

Choose a reason for hiding this comment

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

I like this update!

Just checked and it seems like there's one replacements that's been left out?

image

poetry.lock Outdated Show resolved Hide resolved
@amogus07
Copy link
Contributor Author

amogus07 commented Aug 25, 2024

I like this update!

Just checked and it seems like there's one replacements that's been left out?

image

Which types should be used here?

@amogus07
Copy link
Contributor Author

uhhhh… can't reproduce this locally?!

@amogus07
Copy link
Contributor Author

@snejus What should I do now? Can you reproduce it?

@snejus
Copy link
Member

snejus commented Aug 26, 2024

dict is only supported from Python 3.9 onwards, so try using typing.Dict instead. Also, I think you may want to use str type for the key instead of Any, Dict[str, Any].

@snejus What should I do now? Can you reproduce it?

You should be able to reproduce it in Python 3.8.

@amogus07 amogus07 requested a review from snejus August 26, 2024 22:24
Copy link
Member

@snejus snejus left a comment

Choose a reason for hiding this comment

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

Ideally we want to keep this PR focused on the namedtuple replacement, so I asked to submit the rest of the type changes as a separate PR.

beets/dbcore/db.py Outdated Show resolved Hide resolved
beets/plugins.py Outdated Show resolved Hide resolved
@@ -665,7 +664,7 @@ def show_match_tracks(self):
"""
# Tracks.
# match is an AlbumMatch named tuple, mapping is a dict
# Sort the pairs by the track_info index (at index 1 of the namedtuple)
# Sort the pairs by the track_info index (at index 1 of the NamedTuple)
Copy link
Member

Choose a reason for hiding this comment

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

If you're adjusting this, could you clarify what at index 1 of the NamedTuple means? Can this be replaced by the name of the namedtuple attribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the person who wrote this would have to explain what it means…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JOJ0 can you help out?

beets/util/__init__.py Outdated Show resolved Hide resolved
beets/autotag/match.py Outdated Show resolved Hide resolved
@amogus07 amogus07 requested a review from snejus August 27, 2024 08:27
Copy link
Member

@snejus snejus left a comment

Choose a reason for hiding this comment

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

Thanks for making the requested adjustments, it all looks good now!

@snejus snejus merged commit cd93476 into beetbox:master Aug 27, 2024
12 checks passed
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