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.

Sign up for GitHub

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

Jump to bottom

Fix sorting on missing non-string fields #5570

Merged
merged 10 commits into from
Jan 1, 2025
Merged

Fix sorting on missing non-string fields #5570

merged 10 commits into from
Jan 1, 2025

Conversation

valrus
Copy link
Contributor

@valrus valrus commented Dec 29, 2024

Description

Fixes #5512. When sorting on a field, if the field is missing from some items and it has a type, use the type's null value. Otherwise, continue to fall back to an empty string, as I don't think there's much to be done in that case.

The new test test_int_field_present_in_some_items fails without the fix in query.py.

  • Documentation.
  • Changelog.
  • Tests.

@valrus valrus changed the title Fix sorting on missing int fields Fix sorting on missing non-string fields Dec 29, 2024
snejus
snejus requested changes Dec 31, 2024
View reviewed changes
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 this, I tested it and it works fine! Added a couple of comments

beets/dbcore/query.py Outdated Show resolved Hide resolved
test/test_sort.py Outdated Show resolved Hide resolved
test/test_sort.py Outdated Show resolved Hide resolved
@snejus
Copy link
Member

snejus commented Jan 1, 2025

Looks great. I see that mypy complains about missing mock stubs. Could you by any chance add types-mock to testing dependencies?

@valrus
Copy link
Contributor Author

valrus commented Jan 1, 2025

Could you by any chance add types-mock to testing dependencies?

I'm guessing you meant typing dependencies and added it there instead, but let me know if I misunderstood. There are a bunch of typecheck errors now but none of them are in the changed files. Is something wrong or is this expected?

@snejus
Copy link
Member

snejus commented Jan 1, 2025

Could you by any chance add types-mock to testing dependencies?

I'm guessing you meant typing dependencies and added it there instead, but let me know if I misunderstood.

You're completely correct! Thanks

There are a bunch of typecheck errors now but none of them are in the changed files. Is something wrong or is this expected?

That's expected - mypy should only check the changed files but it ends up reporting issues in related files for some reason 🤷🏼‍♂️ but that's to be sorted separately

snejus
snejus approved these changes Jan 1, 2025
View reviewed changes
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.

Good stuff, thanks for this!

@snejus snejus merged commit f91f096 into beetbox:master Jan 1, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snejus snejus snejus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

mpdstats: Error when querying play_count with a sort order
2 participants
@valrus @snejus