search

maxnowack / node-podcast

Podcast feed generator for Node.
MIT License
157 stars 39 forks source link

Duration should be formatted HH:MM:SS #39

Closed Teun closed 4 years ago

Teun commented 4 years ago

According to the itunes docs, the itunes:duration property should be formatted HH:MM:SS. The readme of this library states (unlike the test code), that it expects the number of seconds as a number. The typescript bindings are also a number. I propose to keep everything the same, but format the number into the right HH:MM:SS string.

maxnowack commented 4 years ago

Thank you for the PR @Teun You're absolutely correct that this is a bug in the current implementation. But since this would be a breaking change for all users who are currently using the duration as a string, it would be nice if you could add a check and only apply the calculation if a number is given as the duration. Can you please adjust the code, that a string would be passed through and a number gets converted to the HH:MM:SS format?

Teun commented 4 years ago

Right. Good point. I will do that.

Op ma 8 jun. 2020 17:50 schreef Max Nowack notifications@github.com:

Thank you for the PR @Teun https://github.com/Teun You're absolutely correct that this is a bug in the current implementation. But since this would be a breaking change for all users who are currently using the duration as a string, it would be nice if you could add a check and only apply the calculation if a number is given as the duration. Can you please adjust the code, that a string would be passed through and a number gets converted to the HH:MM:SS format?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/maxnowack/node-podcast/pull/39#issuecomment-640714296, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADYQT6ZSBLDQ3ARXR3QTQLRVUCEJANCNFSM4NUDQ2XA .

2color commented 4 years ago

Is there any reason not to expect the input to be a string in the format HH:MM:SS?

Teun commented 4 years ago

Well, the README of the project stated that a number (of seconds) was expected and it seems useful that you can pass either. The RSS defacto spec requires a string of HH:MM:SS, so our output should always be that. On the input side we can be more helpful. The users of our library are partly using plain javascript and are probably passing a string (but we don't know). Typescript users used to get an error if they tried that, because the binding specified number. Who knows what to expect? Myself, I pass a number, because the library I use to inspect MP3 files returns me the duration as a number in seconds.

Teun commented 4 years ago

Actually, we should probably update the bindings on definitlytyped to accept (number|string) for duration, as some trypescript users will want to directly specify the HH:MM:SS.

2color commented 4 years ago

Typescript users used to get an error if they tried that, because the binding specified number.

Indeed that was my problem.

Who knows what to expect? Myself, I pass a number, because the library I use to inspect MP3 files returns me the duration as a number in seconds.

I'm curious, which library do you use to inspect MP3 files?

Actually, we should probably update the bindings on definitlytyped to accept (number|string) for duration, as some trypescript users will want to directly specify the HH:MM:SS.

That's a good idea. Is there any reason not to have the bindings in the library?

2color commented 4 years ago

PR created: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/46587

Teun commented 4 years ago

I'm curious, which library do you use to inspect MP3 files?

To get metadata from mp3 files, I use npm modules mp3tag and music-metadata. For the length in seconds, it is music-metadata.

Is there any reason not to have the bindings in the library?

Not really, I think. Max didn't use typescript, others created the binding on definitelytyped, following Max' erroneous documentation comment.