Join GitHub today
GitHub is home to over 36 million developers working together to host and review code, manage projects, and build software together.
Sign upFix some for each cases #896
Conversation
This comment has been minimized.
This comment has been minimized.
I tried to perform all possible tests (however some translator seem broken). Please have a look at the suggested changes. I can update the |
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
Also, note that there are some intricate differences between |
This comment has been minimized.
This comment has been minimized.
Actually, I began with normal
Would that be nicer than the automatic replacement and compatible along all browsers? |
This comment has been minimized.
This comment has been minimized.
Yes, that would be better (except for the typo in |
This comment has been minimized.
This comment has been minimized.
Okay, I can work on that further manually. |
This comment has been minimized.
This comment has been minimized.
I just updated the first ones, i.e.
|
This comment has been minimized.
This comment has been minimized.
Okay, here are the other cases, except the three RDF translators (and IEEE Xplore translator). Is this okay for you to review or do you wish some other action before? Some more |
This comment has been minimized.
This comment has been minimized.
I would suggest to sqash now to have one clean commit for you to review? |
This comment has been minimized.
This comment has been minimized.
@simonster @aurimasv Any opinion here? How to proceed? |
aurimasv
reviewed
Jun 10, 2015
if(censusNos[i] == year) { censusYear = 1 } else {censusYear= 0 }; | ||
for(var j in censusNos) { | ||
if(censusYear == 1) { censusNo = censusNos[j] }; | ||
if(censusNos[j] == year) { censusYear = 1 } else {censusYear= 0 }; |
This comment has been minimized.
This comment has been minimized.
aurimasv
Jun 10, 2015
Contributor
All of this should just be a year -> census number map. The whole loop is unnecessary.
This comment has been minimized.
This comment has been minimized.
zuphilip
Jun 10, 2015
Author
Collaborator
I am not sure whether I should change something her now AND what I should change this exactly.
aurimasv
reviewed
Jun 10, 2015
"translatorID": "0dda3f89-15de-4479-987f-cc13f1ba7999", | ||
"label": "Ancestry.com US Federal Census", | ||
"creator": "Elena Razlogova", | ||
"target": "^https?://search.ancestry.com/(.*)usfedcen|1890orgcen|1910uscenindex", |
This comment has been minimized.
This comment has been minimized.
aurimasv
Jun 10, 2015
Contributor
this target regexp is wrong. Should be "^https?://search.ancestry.com/.*(usfedcen|1890orgcen|1910uscenindex)"
aurimasv
reviewed
Jun 10, 2015
if (data['FechaEdicin']) item.date = Zotero.Utilities.trimInternal(data['FechaEdicin']); | ||
item.complete(); | ||
} | ||
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
zuphilip
Jun 10, 2015
Author
Collaborator
I sometimes had problems to read the code because the whitespacing was wrong and therefore I tried to improve a little. Is this in general a bad idea or are you more concenrned about the git history?
aurimasv
reviewed
Jun 10, 2015
if(temp.value.indexOf("Story")>-1 || temp.value=="Blog") { | ||
scrape(doc,url); | ||
} | ||
} |
This comment has been minimized.
This comment has been minimized.
aurimasv
reviewed
Jun 10, 2015
@@ -95,4 +98,4 @@ var testCases = [ | |||
"items": "multiple" | |||
} | |||
] | |||
/** END TEST CASES **/ | |||
/** END TEST CASES **/ |
This comment has been minimized.
This comment has been minimized.
aurimasv
Jun 10, 2015
Contributor
since we're undoing whitespace, might as well undo this one and the one above.
aurimasv
reviewed
Jun 10, 2015
@@ -151,7 +151,7 @@ function extractCitation(url, elmts, title, doc) { | |||
|
|||
// ensure author is not already there | |||
var add = true; | |||
for each(var existingAuthor in newItem.creators) { | |||
for (let existingAuthor of newItem.creators) { |
This comment has been minimized.
This comment has been minimized.
aurimasv
Jun 10, 2015
Contributor
can't use let
in translators, since it's currently only supported in Firefox
This comment has been minimized.
This comment has been minimized.
aurimasv
Jun 10, 2015
Contributor
Also this one lacks timestamp bump and is different from other changes.
aurimasv
reviewed
Jun 10, 2015
for each (var aut in authors) { | ||
aut = aut.replace(/[^\w^\s^\.]/g, "").replace(/\d/g, ""); | ||
for (var i=0; i<authors.length; i++) { | ||
var aut = authors[i].replace(/[^\w^\s^\.]/g, "").replace(/\d/g, ""); |
This comment has been minimized.
This comment has been minimized.
aurimasv
Jun 10, 2015
Contributor
[^\w^\s^\.]
not entirely sure what this is supposed to mean, but it looks like this should be just var aut = authors[i].replace(/[^A-Z\s.]+/gi);
This comment has been minimized.
This comment has been minimized.
zuphilip
Jun 10, 2015
Author
Collaborator
I don't understand. Replaced with empty string? Only this one replace or also the second replace from the line above? Should I really change something here? I just undo all space improvements ...
aurimasv
reviewed
Jun 10, 2015
}/** BEGIN TEST CASES **/ | ||
} | ||
|
||
/** BEGIN TEST CASES **/ |
This comment has been minimized.
This comment has been minimized.
aurimasv
reviewed
Jun 10, 2015
// fix capitalization | ||
var words = author.split(" "); | ||
for (var i in words) { | ||
words[i] = words[i][0].toUpperCase() + words[i].substr(1).toLowerCase(); | ||
for (var k in words) { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
zuphilip
Jun 10, 2015
Author
Collaborator
I didn't touch any of the for ... in
except for variable renaming. I thought that for ... in
is not deprecated and compatible among all browsers. Am I wrong?
aurimasv
reviewed
Jun 10, 2015
@@ -165,8 +166,8 @@ function scrape(doc, url) { | |||
if (metaTags["keywords"]) { | |||
var keywords = metaTags["keywords"]; | |||
newItem.tags = keywords.split(","); | |||
for (var i in newItem.tags) { | |||
newItem.tags[i] = newItem.tags[i].replace(" ", ", "); | |||
for (var t in newItem.tags) { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
aurimasv
Jun 10, 2015
Contributor
nit: Also, all of these can be i
, unless they share the same scope.
This comment has been minimized.
This comment has been minimized.
zuphilip
Jun 10, 2015
Author
Collaborator
This may be a little too much caution, but yes they are in the same function which is the scope for var
.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
zuphilip
Jun 10, 2015
Author
Collaborator
How about double variable declaration? I.e. something like
for (var i=0; ....) {
}
for (var i=0; ...) {
}
AFAIK this is still okay, but maybe you have a coding convention on these...
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
dstillman
Jun 10, 2015
Member
No, var
for both. (Doesn't really matter in translators, since they're in a sandbox, but otherwise if the first one gets removed the second becomes a global assignment, which would trigger an error in strict mode.)
aurimasv
reviewed
Jun 10, 2015
@@ -260,7 +260,7 @@ function doWeb(doc, url) { | |||
//extract PMID from a context object | |||
function getPMID(co) { | |||
var coParts = co.split("&"); | |||
for each(part in coParts) { | |||
for (let part of coParts) { |
This comment has been minimized.
This comment has been minimized.
aurimasv
reviewed
Jun 10, 2015
@@ -278,7 +278,7 @@ function doExport() { | |||
// tags | |||
if(item.tags) { | |||
var keywordTag = ""; | |||
for each(var tag in item.tags) { | |||
for (let tag of item.tags) { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
aurimasv
reviewed
Jun 10, 2015
"translatorID": "6f5f1b24-7519-4314-880f-d7004fbcfe7e", | ||
"label": "ReliefWeb", | ||
"creator": "Michael Berkowitz", | ||
"target": "http://(www.)?reliefweb.int/", |
This comment has been minimized.
This comment has been minimized.
aurimasv
reviewed
Jun 10, 2015
sel_items[i.url] = i.title; | ||
sel_items = Zotero.selectItems(sel_items); | ||
|
||
for (var i in sel_items) | ||
items[i].complete(); | ||
} else if (post_count == 1) | ||
for each(var i in items) | ||
for (let i of items) |
This comment has been minimized.
This comment has been minimized.
aurimasv
reviewed
Jun 10, 2015
"translatorID": "e0234bcf-bc56-4577-aa94-fe86a27f6fd6", | ||
"label": "The Globe and Mail", | ||
"creator": "Adam Crymble", | ||
"target": "http://www.theglobeandmail.com", |
This comment has been minimized.
This comment has been minimized.
aurimasv
reviewed
Jun 10, 2015
for (var i in words) { | ||
if (words[i] != "") { | ||
words[i] = words[i][0].toUpperCase() + words[i].substr(1).toLowerCase(); | ||
for (var j in words) { |
This comment has been minimized.
This comment has been minimized.
aurimasv
reviewed
Jun 10, 2015
@@ -86,7 +86,7 @@ function getData(ids){ | |||
var keywords; | |||
if ((keywords = ZU.xpathText(doc, '//media:group/media:keywords', ns))) { | |||
keywords = keywords.split(","); | |||
for each(var tag in keywords){ | |||
for (let tag of keywords){ |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
just dropping in quickly to point out that some of these translators are entirely defunct (e.g. Ancestry afaik) so it doesn't make any sense to fix details of them. |
This comment has been minimized.
This comment has been minimized.
@adam3smith When do you delete a translator completely from the repo? An "entirely defunct translator" sounds for me like we should delete that. When there is some interest in the future, it might be possible to write a new one. |
This comment has been minimized.
This comment has been minimized.
we delete translators when sites don't exist anymore or translators are merged. We don't delete translators that are completely broken if there's any chance they'll be replaced by functioning versions in the future. |
This comment has been minimized.
This comment has been minimized.
Hm... it would at least be good to "mark" such completely broken translator. If "code may still be useful" then I suggest also to do these refactoring on this code as well. |
zuphilip
added some commits
Jun 10, 2015
This comment has been minimized.
This comment has been minimized.
@aurimasv I updated the PR as you requested. Please have a look at the new version. |
zuphilip
reviewed
Jun 10, 2015
var byline = doc.evaluate('//div[@class="article_author"]|//span[contains(@class,"article-byline")]', doc, null, XPathResult.ANY_TYPE, null); | ||
var authors = byline.iterateNext().textContent.replace(/^by\s*/i, "").split(" and "); | ||
for (var i=0; i<authors.length; i++) { | ||
newItem.creators.push(Zotero.Utilities.cleanAuthor(authors[i], "author")); |
This comment has been minimized.
This comment has been minimized.
zuphilip
Jun 10, 2015
Author
Collaborator
I fixed some of the regexp for the test case. Should I make a seperate pull request for that (i.e. exclude this translator here)?
aurimasv
merged commit a393202
into
zotero:master
Jun 17, 2015
aurimasv
added a commit
that referenced
this pull request
Jun 17, 2015
aurimasv
added a commit
that referenced
this pull request
Jun 17, 2015
This comment has been minimized.
This comment has been minimized.
Thanks!! (though I probably should have squashed those commits on merge. Ooops) |
zuphilip
deleted the
zuphilip:refactor-for-eachs
branch
Jun 17, 2015
This comment has been minimized.
This comment has been minimized.
Okay. Cool. Merging would have been done a cleaner git history, but in this way my stats (+9 commits) are evolving faster ;-) Now, we only should have |
This comment has been minimized.
This comment has been minimized.
Should be able to do a straightforward replacement from
Though I haven't looked at those cases in particular |
zuphilip commentedJun 1, 2015
match
,split
,Zotero.RDF.getStatementsMatching
).