-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Conversation
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uhhhh… can't reproduce this locally?! |
@snejus What should I do now? Can you reproduce it? |
You should be able to reproduce it in Python 3.8. |
There was a problem hiding this 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.
@@ -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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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…
There was a problem hiding this comment.
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?
This reverts commit 647d6ab.
There was a problem hiding this 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!
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.