Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upIGN translator rewritten in Framework #1080
Conversation
zuphilip
reviewed
Jun 15, 2016
| @@ -1,103 +1,105 @@ | |||
| { | |||
| "translatorID": "d210c5a1-73e1-41ad-a3c9-331d5a3ead48", | |||
| "translatorID": "cecafe6f-7fc9-4bdb-ae70-bd0a75a8ca52", | |||
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
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. |
This comment has been minimized.
This comment has been minimized.
|
Why? This performs the same function but isn't broken and is easier for others to maintain |
This comment has been minimized.
This comment has been minimized.
|
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. |
This comment has been minimized.
This comment has been minimized.
|
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
force-pushed the
owcz:june-updates
branch
from
e4312eb
to
0053b19
Jun 15, 2016
adam3smith
reviewed
Jun 15, 2016
| } | ||
| FW.Scraper({ | ||
| itemType : 'blogPost', | ||
| detect : FW.Url().match(/articles\/\d{4}/), |
This comment has been minimized.
This comment has been minimized.
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)
owcz
force-pushed the
owcz:june-updates
branch
from
e75c6ba
to
0a18bf3
Jun 15, 2016
adam3smith
merged commit fc8e910
into
zotero:master
Jun 15, 2016
This comment has been minimized.
This comment has been minimized.
|
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. |
This comment has been minimized.
This comment has been minimized.
|
Didn't know you could do that (I only squashed since that's what you wanted last time)—noted & thanks! |
owcz
deleted the
owcz:june-updates
branch
Jun 15, 2016
added a commit
to zuphilip/translators
that referenced
this pull request
Mar 28, 2018
added a commit
to zuphilip/translators
that referenced
this pull request
Mar 28, 2018
owcz commentedJun 15, 2016
Rewritten in Framework, replacing antiquated, outdated version