Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: Decoding done with iconv-lite, support for all idv3 text encoding types #4

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

peerbelt-build
Copy link

No description provided.

@tmont
Copy link
Owner

tmont commented Feb 12, 2017

I'm not opposed in theory to either adding more support for encoding or adding more ID3v2 tags. However, there are a few goals that this library aims for:

  1. Smallish file size
  2. Speed

1 means there can be no external dependencies (and also because this needs to run in the browser).

Also, there are a lot of ID3v2 tags that can be supported; I don't necessarily want this library to handle all of them (due to the file size issue). By default it handles the most common ones, which I think is enough. Things like official_internet_radio_station_url are overkill for this library. If someone wants to handle all of those things, there are other more complete libraries for that.

So in order for me to accept this pull request, you would need to do the following:

  1. Ensure there are no dependencies
  2. Remove the added ID3v2 tags
    • I could be persuaded to keep some of them in if there are good reasons for it
  3. Add lots of encoding tests
  4. Fix small coding style issues, e.g. if ( foo ) {... should be if (foo) {.... I know there's no style linter, but at least making the spacing consistent will be enough.

If you think that's stupid, I understand. This library has very specific goals that I want to adhere to that may not be in line with what you need. If you want to keep your fork with more robust tag parsing, let me know where you put it and I will be happy to put a link to it in the README.

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