ghaiklor / icecast-parser

Node.js module for getting and parsing metadata from SHOUTcast/Icecast radio streams
MIT License
70 stars 18 forks source link

Change NotifyOnChangeOnly to accept a list #199

Open Beastlybear2017 opened 12 months ago

Beastlybear2017 commented 12 months ago

I apologise if this is really dumb, I have never attempted to contribute towards a npm package before and have no clue how to / if it is possible to test it from a local file.

Here is my vision of how this would work, although have no clue if it works in practice :)

ghaiklor commented 11 months ago

@Beastlybear2017 you can try to write a test, where to check if your solutions works.

Here, for instance, test case that checks if the notifyChange works:

https://github.com/ghaiklor/icecast-parser/blob/e6ba7a3cb7161eb0c9ce08039533fa5f8afa1dda/test/Parser.spec.ts#L57-L65

You can add more cases like that where you check scenarios like “what if you provided no keys at all, that way it should notify every time” or “what if I provided the keys that doesn't exist in metadata, it should notify” and so on.

Beastlybear2017 commented 11 months ago

I have added the tests (updated yours to use the new system), fixed any issues and commited my changes. All the tests pass / fail when they should https://github.com/Beastlybear2017/icecast-parser/blob/ccab52975b0fdf5586ff5734e9a171cdcffe779a/test/Parser.spec.ts#L68-L95

image

Beastlybear2017 commented 11 months ago

Was able to test within my project and fixed an issue that I discorvered. Now works as intened

ghaiklor commented 11 months ago

Great work!

However, let's keep it backward compatible with older API. Let's leave the possibility to specify the boolean as before.