Uploaded image for project: 'Picard'
  1. Picard
  2. PICARD-1406

Refactor Metadata.set and metadata.__setitem__

XMLWordPrintable

    • Icon: Task Task
    • Resolution: Fixed
    • Icon: Normal Normal
    • 2.3.0b1
    • 2.0.4
    • Tags & Metadata
    • None

      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:

      1. Make set and _setitem_ be the same
      2. Make add and add_unique also do the str conversion for consistency
      3. 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

            outsidecontext Philipp Wolfer
            outsidecontext Philipp Wolfer
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

              Created:
              Updated:
              Resolved:

                Version Package
                2.3.0b1