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

Added barcode field #27

Merged
merged 4 commits into from
Dec 15, 2019
Merged

Added barcode field #27

merged 4 commits into from
Dec 15, 2019

Conversation

SimonHova
Copy link
Contributor

I added the barcode field to clear issue #26.

@sampsyo
Copy link
Member

sampsyo commented Dec 13, 2019

Looks great so far! We'll need to do a couple of things:

  • Update the tests to include the new field.
  • Double-check the new tag mappings against existing software, especially the Picard tag mappings.

@SimonHova
Copy link
Contributor Author

The tag mappings look pretty straightforward, with the exception of the iTunes. But, I matched with another field, and it looks good now. Tell me what you think.

@sampsyo
Copy link
Member

sampsyo commented Dec 14, 2019

Looks pretty good to me, and it seems to match the Picard mappings! Can you elaborate on what was less straightforward about the iTunes (MPEG-4) tag?

@SimonHova
Copy link
Contributor Author

Sure. All the other tags had the name barcode, but the iTunes one had six dashes instead. I'm presuming that means that iTunes doesn't support the field, but I copied the format which had an identical format from another field, so I'm confident that it's correct.

@sampsyo
Copy link
Member

sampsyo commented Dec 14, 2019

Just checking, do you mean four dashes, as in ----:com.apple.iTunes:BARCODE? That is indeed what Picard seems to use, which is a good indication that it's also what other software uses. (It's worth mentioning that compatibility with other software is the paramount concern.)

@SimonHova
Copy link
Contributor Author

That is absolutely correct, I copied the format from another field that had a similar notation in the table.

@sampsyo sampsyo merged commit 0cfec87 into beetbox:master Dec 15, 2019
sampsyo added a commit that referenced this pull request Dec 15, 2019
@sampsyo
Copy link
Member

sampsyo commented Dec 15, 2019

Perfect; thanks!! Merged. ✨

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