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

Add unit test and sample for UNSYNCEDLYRICS vorbis tag #1636

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

Conversation

Vorelli
Copy link

@Vorelli Vorelli commented Apr 14, 2023

No description provided.

@Vorelli
Copy link
Author

Vorelli commented Jun 13, 2023

Could you please merge this? Or is there something else I need to take care of?

@Borewit Borewit self-assigned this Jun 13, 2023
Copy link
Owner

@Borewit Borewit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution @Vorelli, in principal a good change.

Please update the PR title and description.
The PR title is used in the change-log, to summarize this improvement, as port of the next release, to the user. In the description (initial comment) you can be more specific. If possible link to UNSYNCEDLYRICS documentation or other reference material describing this tag.

Advice for future PR's, do not no use your master branch to create a PR. That prevents you from syncing changes from this master in your local clone.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use a much smaller sample, truncate the audio content to a minimum.

yarn.lock Outdated
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change should not be part of this PR.

flacFilePath,
"Sister City - Swan Dive - 01 Attenborough Blues.flac"
);
const { format, common, native } = await mm.parseFile(filePath);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not used

assert.strictEqual(format.container, "FLAC", "format.container");
assert.strictEqual(format.codec, "FLAC", "format.codec");

assert.strictEqual(common.lyrics instanceof Array && common.lyrics[0] && common.lyrics[0].length > 0, true);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be verbose and explicit unit test, provide clear message where and what is wrong in case of failing unit test.

Suggested change
assert.strictEqual(common.lyrics instanceof Array && common.lyrics[0] && common.lyrics[0].length > 0, true);
assert.isArray(common.lyrics, 'common.lyrics');
assert.isNotEmpty(common.lyrics, 'common.lyrics');

"A primate blessed with common sense\r\n" +
"With empathy and with arrogance\r\n" +
"I am swinging from the trees\r\n" +
"I am crawling out of the sea"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Include the metadata field name you are checking:

Suggested change
"I am crawling out of the sea"
"I am crawling out of the sea", 'common.lyrics');

@Vorelli Vorelli force-pushed the master branch 2 times, most recently from 749649b to 1555826 Compare October 2, 2023 03:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants