Skip to content

Conversation

@pinoaffe
Copy link
Contributor

@pinoaffe pinoaffe commented Jun 2, 2025

No description provided.

Copy link
Contributor

@DamienCassou DamienCassou left a comment

Choose a reason for hiding this comment

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

Thank you for decomposing your PR. Can you please have a look at my feedback? Is there anything to update in the README?

@pinoaffe
Copy link
Contributor Author

pinoaffe commented Jun 3, 2025

and as to the readme: This does not introduce any new data types, and since many of the fields of libmpdel-song are not listed I don't think it makes sense to list the performers field

Copy link
Contributor

@DamienCassou DamienCassou left a comment

Choose a reason for hiding this comment

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

This looks good to me. Can we add a unit test or two?

As for #17, please sign your commits if you can.

@pinoaffe pinoaffe requested a review from DamienCassou June 5, 2025 12:17
Copy link
Contributor

@DamienCassou DamienCassou left a comment

Choose a reason for hiding this comment

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

Excellent. Do you plan to sign your commits or should I move forward without?

@pinoaffe
Copy link
Contributor Author

pinoaffe commented Jun 5, 2025

I plan to sign them, just have to figure out how to set that up

@DamienCassou
Copy link
Contributor

I plan to sign them, just have to figure out how to set that up

you may want to read this: https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits

@pinoaffe
Copy link
Contributor Author

pinoaffe commented Jun 6, 2025

Thanks for the link, I signed the commits

Copy link
Contributor

@DamienCassou DamienCassou left a comment

Choose a reason for hiding this comment

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

Excellent! Thank you

@DamienCassou DamienCassou merged commit d299cac into mpdel:master Jun 9, 2025
1 check 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