search

tastejs / TasteMusic

Empowering your MDD (Music Driven Development) - Music recommendations by JavaScripters
https://github.com/tastejs/TasteMusic/issues/1
178 stars 13 forks source link

General improvements to CommentsParser #10

Closed vasilionjea closed 10 years ago

Glavin001 commented 10 years ago

Should we use @sindresorhus's https://github.com/sindresorhus/get-urls for retrieving URLs from the comments string?

vasilionjea commented 10 years ago

Feel free to send a pull request if you think it'd be better.

Glavin001 commented 10 years ago

I'm not really sure; I was hoping @sindresorhus would be able to describe the benefits and if he think it's worth while. Just noticed it in my feed.

One thing that is interesting and useful is that it would " normalized and uniquified" the URLs. Is that something that is already being done with your commentParser? And do we need it? Will the various music service's API care if our URLs are not normalized?

I will take a closer look and of course make a PR if it's worth while. Your script is working well, and quick as is, so it is a likely unnecessary change.

vasilionjea commented 10 years ago

I think whether it proves to be an issue or not, URL normalization is definitely worth doing. I would think it would be a good practice in general.

As for making the URLs unique, with Underscore it's as easy as _.uniq(array);... but yeah I haven't done that yet.

sindresorhus commented 10 years ago

I don't care either way, but I definitely think we shouldn't reinvent the wheel. Use small reusable libs instead of doing it yourself ;)

vasilionjea commented 10 years ago

I'm open to using any libs out there as long as we define why we're using x option -vs- y option. Either way the main goal is to build an awesome/useful app, so personally I don't care.

sindresorhus commented 10 years ago

@vasilionjea modularity and NIH

vasilionjea commented 10 years ago

NIH?

sindresorhus commented 10 years ago

@vasilionjea Not Invented Here (google it)

Glavin001 commented 10 years ago

Would @sindresorhus or @addyosmani be able to merge this PR when they get a chance?

I'm hopefully going to have time tonight to get one of the players working -- probably start with SoundCloud (See #5). Then we will have something working to encourage developers to contribute to.

@sindresorhus, did you have a chance to look into publishing this to gh-pages branch or some other server? There's not much to see right now, however hopefully there will be soon.

sindresorhus commented 10 years ago

Landed :D

Will get it published later today.

Glavin001 commented 10 years ago

Excellent, thank you!

vasilionjea commented 10 years ago

Thanks for the "google it" recommendation @sindresorhus! Now I know... LOL!

sindresorhus commented 10 years ago

@Glavin001 done: http://tastejs.com/TasteMusic/

Glavin001 commented 10 years ago

Thanks, @sindresorhus!