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

IGN translator rewritten in Framework #1080

Merged
merged 1 commit into from Jun 15, 2016

Conversation

Projects
None yet
4 participants
@owcz
Copy link
Contributor

owcz commented Jun 15, 2016

Rewritten in Framework, replacing antiquated, outdated version

IGN.js Outdated
@@ -1,103 +1,105 @@
{
"translatorID": "d210c5a1-73e1-41ad-a3c9-331d5a3ead48",
"translatorID": "cecafe6f-7fc9-4bdb-ae70-bd0a75a8ca52",

This comment has been minimized.

Copy link
@zuphilip

zuphilip Jun 15, 2016

Collaborator

You should never change an existing translatorID for a translator, if you are updating it.

This comment has been minimized.

Copy link
@owcz

owcz Jun 15, 2016

Author Contributor

Even if it completely replaces the former translator, without re-using any of it?

This comment has been minimized.

Copy link
@adam3smith

adam3smith Jun 15, 2016

Collaborator

yes -- otherwise you won't replace the old translator.

@zuphilip

This comment has been minimized.

Copy link
Collaborator

zuphilip commented Jun 15, 2016

I haven't looked at the details, but usually I think that a normal translator would be preferred compared to one that is using the framework.

@owcz

This comment has been minimized.

Copy link
Contributor Author

owcz commented Jun 15, 2016

Why? This performs the same function but isn't broken and is easier for others to maintain

@avram

This comment has been minimized.

Copy link
Contributor

avram commented Jun 15, 2016

Probably the wrong forum for this, but I see little reason to discourage Framework translators that work. Yes, they're more limited and can't be customized as much. But they can also lower the bar for contribution and help increase the total coverage that our translators have.

@adam3smith

This comment has been minimized.

Copy link
Collaborator

adam3smith commented Jun 15, 2016

agree with @avram here: As long as we don't need any clean-up on a translator, I don't see why we shouldn't do it as FW.

@owcz owcz force-pushed the owcz:june-updates branch from e4312eb to 0053b19 Jun 15, 2016

IGN.js Outdated
}
FW.Scraper({
itemType : 'blogPost',
detect : FW.Url().match(/articles\/\d{4}/),

This comment has been minimized.

Copy link
@adam3smith

adam3smith Jun 15, 2016

Collaborator

given that you only detect on ign.com/articles/, could you add /articles/ to the target (leave it in detect to be safe)

IGN translator rewritten in FW
Rewritten in the Framework, replacing antiquated, outdated version

@owcz owcz force-pushed the owcz:june-updates branch from e75c6ba to 0a18bf3 Jun 15, 2016

@adam3smith adam3smith merged commit fc8e910 into zotero:master Jun 15, 2016

@adam3smith

This comment has been minimized.

Copy link
Collaborator

adam3smith commented Jun 15, 2016

Thanks, looks good. FWIW, I think it makes it easier to review if you don't squash your commits, especially now that we can just squash on merging.

@owcz

This comment has been minimized.

Copy link
Contributor Author

owcz commented Jun 15, 2016

Didn't know you could do that (I only squashed since that's what you wanted last time)—noted & thanks!

@owcz owcz deleted the owcz:june-updates branch Jun 15, 2016

zuphilip added a commit to zuphilip/translators that referenced this pull request Mar 28, 2018

IGN translator rewritten in FW (zotero#1080)
Rewritten in the Framework, replacing antiquated, outdated version

zuphilip added a commit to zuphilip/translators that referenced this pull request Mar 28, 2018

IGN translator rewritten in FW (zotero#1080)
Rewritten in the Framework, replacing antiquated, outdated version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.