Skip to content

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

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