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

PICARD-3000: Children's Music is shown as "Children'S Music" in Picard #2548

Merged
merged 6 commits into from
Nov 5, 2024

Conversation

zytact
Copy link
Contributor

@zytact zytact commented Nov 5, 2024

Summary

  • This is a…
    • Bug fix
    • Feature addition
    • Refactoring
    • Minor / simple change (like a typo)
    • Other
  • Describe this change in 1-2 sentences:

Problem

Genre Children's Music is shown as Children'S Music using Python's title method.

Solution

Create a custom function to handle making the genre title case which also handles the apostrophe.

Copy link
Member

@phw phw 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 patch. For this use case it should work. But I wonder if we should use a better function to begin with.

There is an existing title case function as scripting at https://github.com/metabrainz/picard/blob/master/picard/script/functions.py#L964

I think we should factor out the title casing functionality into a function in picard.util, e.g. just def titlecase(s). Then this function can be used both here for handling the genres and in the func_title scripting function.

This also allows to add tests for the title case function.

@phw phw requested a review from zas November 5, 2024 10:34
@zytact
Copy link
Contributor Author

zytact commented Nov 5, 2024

Thank you for looking into it. I think that would be best way to do it. If we use this approach should I create another separate PR for handling that?

@phw
Copy link
Member

phw commented Nov 5, 2024

@zytact Would be allright to do this as part of this PR

zas
zas previously approved these changes Nov 5, 2024
Copy link
Collaborator

@zas zas left a comment

Choose a reason for hiding this comment

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

Nice fix & code improvement, LGTM.

Copy link
Member

@phw phw left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this contribution. Looks great. Just the initial titlize function added to track.py can now be removed. Then this is good to merge

picard/track.py Outdated Show resolved Hide resolved
Copy link
Member

@phw phw left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM

@zas zas merged commit 4478043 into metabrainz:master Nov 5, 2024
44 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.

3 participants