I think from an API side it is actually pretty bad that we have both metadata[t] = v and metadata.set(t, v) which look so much alike but behave absolutely different. Furthermore the special behavior of metadata.set (namely allowing arbitrary values to be set, including empty, without str conversion) is currently not used by core Picard nor the existing plugins. There are some instances of metadata.set calls, but they mainly ensure data is copied 1:1, but themselves don't cause e.g. empty values to be added.
Also the reading methods metadata.get(t) and metadata[t] behave exactly the same, so its inconsistent to have the setters behave different.
But I was not brave enough to change it. Actually it was my first approach to unify both methods, but it is an API breaking change after all. What are your opinions?
I personally would refactor the Metadata class as follows:
- Make set and _setitem_ be the same
- Make add and add_unique also do the str conversion for consistency
- Optional: If needed introduce some functions setraw and getraw to really set and get raw values without any conversion
See discussion at https://github.com/metabrainz/picard/pull/1027