Allow dashes in music videos on the first pass #2624

Merged
merged 1 commit into from Apr 21, 2013

Conversation

Projects
None yet
5 participants
Contributor

ScudLee commented Apr 16, 2013

This removes the limitation on Music videos to have their dashes removed immediately prior to scraping.

Not sure why this was put in in the first place?

(This will require an update to the scraper before being pulled.)

Contributor

davilla commented Apr 16, 2013

Did you git blame and track down the commit that remove the dashes ? I highly suggest that we research why this was done in the first place or risk regressions. Found it so NAK on this PR :)

ScudLee/xbmc@07bdfd5

fixed: remove hyphens in search-string in case the first lookup gave no results (closes #12357)

Contributor

ScudLee commented Apr 16, 2013

That doesn't go back far enough, the music videos having the dashes removed was part of the initial scraper abstraction:

5a0d0ec Scraper abstraction: provide an encapsulation.

Member

jmarshallnz commented Apr 16, 2013

According to that commit you want to go even further back.

Contributor

ScudLee commented Apr 16, 2013

Found it: 5499661

Contributor

davilla commented Apr 16, 2013

stuff like this should be commented in the code :)

olympia commented Apr 17, 2013

so what is the conclusion? By the way, we are not risking regression here. Lee, Zag and I are trying to fix something (musicvid scraping) and get it to somewhat to a working level what was useless if not completly broken for very long time.

Contributor

ScudLee commented Apr 17, 2013

Based on the trac ticket and the forum thread, I'd say this was only added to solve a very specific problem (possibly connected to the MTV scraper?), and has just been carried forward ever since.

Since there's only currently the TheAudioDb.com scraper now, and it would definitely benefit from the fix...

Member

jmarshallnz commented Apr 17, 2013

Agreed - good detective work :)

Contributor

ScudLee commented Apr 17, 2013

I'd want to do a minor backwards-compatibility commit to the scraper (basically replace %20%20%20 with %20(?:%20|-)%20 ) first, before this is pulled.

Although we can probably drop that if it's suitable for backporting to 12.2 (olympia's suggestion).

Contributor

ScudLee commented Apr 17, 2013

I've pushed the change to the scraper, so feel free to pull this whenever.

olympia commented Apr 21, 2013

Is there still some blocking reason to push this to master? As said in my opinion this sould be pushed to the Frodo branch as well, I do hope you are not against it Davilla :)

Member

jmarshallnz commented Apr 21, 2013

Nothing stopping it hitting master at least.

On Sun, Apr 21, 2013 at 5:06 PM, olympia notifications@github.com wrote:

Is there still some blocking reason to push this to master? As said in my
opinion this sould be pushed to the Frodo branch as well, I do hope you are
not against it Davilla :)


Reply to this email directly or view it on GitHubhttps://github.com/xbmc/xbmc/pull/2624#issuecomment-16716153
.

MartijnKaijser added a commit that referenced this pull request Apr 21, 2013

Merge pull request #2624 from ScudLee/musicvideodashfix
Allow dashes in music videos on the first pass

@MartijnKaijser MartijnKaijser merged commit 273b064 into xbmc:master Apr 21, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment