Add constants for obscure tag values about node-id3 HOT 7 CLOSED

About point 3, I did notice the current particular case for APIC tag frame, here are some comments about it.

The documentation uses the terminology picture but the current implementation uses image as the property name.

The documentation uses the term pictureType but the library currently uses type as the property name
It would have been nicer to align terminology with the documentation as much as possible.

Also to better align with the rest of the library pictureType should just have been a number in both create and read. To better separate concerns, the name could be retrieve separately from the the type. More on this below.

Fixing these now would introduce breaking changes, so I suppose we will leave with it that way. Later we could introduced a new major version with breaking changes.

Some issues the way this is currently implemented:

• the create and read API are asymmetric:
  The picture type is currently hard-coded to 3 (Front Cover) here , but is returned as an object with properties type: { id: number, name: string }.
  I am not sure why the name is returned here, usually a name is used either to display in a UI or as a debug facility. Also names are usually preferred to numbers (easier to work with), but here since we are very close to the format for all the other frames (all the other frames accept and return numbers), I think we should have returned just type: number.

Ideally for APIC we would like to have the read return an object format compatible with the create, imagine, you read all the tags and want to rewrite them (which I will need to do in my code), then you can read => modify/delete/add => write (note that update unfortunately has a particular case for tags with multiple values, I won't extend on that here).

With that in mind, I suggest the following changes:

• provide a set of constant definitions for the picture type
• for create
  • accepts type as an object { id: number } (name should be ignored), accepting the object format allows to make it compatible with the value returned by read, we could also potentially accept type as a number, but that would complicate code for both the library and clients of the library, I leave this open for discussion
• for read
  • nothing to change I believe
• update the documentation to specify that name is informational only provided by read but not accepted by create

from node-id3.

We could potentially also provide maps to map constants to debug names for user convenience (similarly to the APIC case).

from node-id3.

I will try to provide a PR for this after this one #124.

from node-id3.

Hi @pbricout,

you're absolutely correct about the APIC frame, especially the hardcoded cover type. To give some context about the naming choices, the first implementation was commited 6 years ago (1d0a550) and it was honestly a mess itself 😄 I didn't really think much about naming choices etc., just wanted it to work back then. But with more inconsistencies, it's clear that it was a bad choice. I also don't remember why I chose to add the name to the type.

However, I've always tried not to break anything when re-writing things while the library is at version 0.x, which is why I don't think renaming is an option for now, like you said. I wanted to at least get all v2.3.0 frames working before upping to 1.x. I'd probably wait with accepting type as a number until the read implementation can be changed. The hardcoded type is definitely a bug, though.

from node-id3.

What do you think if we simply use Constants instead of TagConstants ? Since this is in the context of a tag library, maybe the tag prefix isn't necessary?

from node-id3.

I have now pushed a PR #126.

from node-id3.

Merged in 17a2479 / ed4495b, thanks!

from node-id3.