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

Add translator for sverigesradio.se #1567

Merged
merged 1 commit into from Mar 4, 2018

Conversation

Projects
None yet
2 participants
@sebastian-berlin-wmse
Contributor

sebastian-berlin-wmse commented Feb 26, 2018

This is the website for the Swedish public service radio.

@adam3smith

Thanks -- looks good in general, some small nits

Show outdated Hide outdated Sveriges radio.js
"translatorID": "caa8f42c-9dbf-446e-963b-6ee18e3133d2",
"label": "Sveriges radio",
"creator": "Sebastian Berlin",
"target": "^https?://sverigesradio\\.se/sida/artikel.aspx",

This comment has been minimized.

@adam3smith

adam3smith Feb 27, 2018

Collaborator

This doesn't seem right -- it wouldn't detect multiples if your code below is correct (please add some multiples as tests, if possible)

@adam3smith

adam3smith Feb 27, 2018

Collaborator

This doesn't seem right -- it wouldn't detect multiples if your code below is correct (please add some multiples as tests, if possible)

This comment has been minimized.

@sebastian-berlin-wmse

sebastian-berlin-wmse Feb 28, 2018

Contributor

Extended regex to include search page.

@sebastian-berlin-wmse

sebastian-berlin-wmse Feb 28, 2018

Contributor

Extended regex to include search page.

Show outdated Hide outdated Sveriges radio.js
item.creators = [];
var nameNodes = ZU.xpath(doc, '//p[@class="byline"]/text()');
for(var node of nameNodes) {

This comment has been minimized.

@adam3smith

adam3smith Feb 27, 2018

Collaborator

let is prefered over var for loops and similar temp uses

@adam3smith

adam3smith Feb 27, 2018

Collaborator

let is prefered over var for loops and similar temp uses

Show outdated Hide outdated Sveriges radio.js
// those.
continue;
}
var names = nameString.split(" ");

This comment has been minimized.

@adam3smith

adam3smith Feb 27, 2018

Collaborator

It seems like you're just generating a crude version of ZU.cleanAuthor here -- I'd recommend using that.

@adam3smith

adam3smith Feb 27, 2018

Collaborator

It seems like you're just generating a crude version of ZU.cleanAuthor here -- I'd recommend using that.

This comment has been minimized.

@sebastian-berlin-wmse

sebastian-berlin-wmse Feb 28, 2018

Contributor

I missed that function. Replaced the logic I had written, but some preprocessing was still required due to how the byline string can be formatted.

@sebastian-berlin-wmse

sebastian-berlin-wmse Feb 28, 2018

Contributor

I missed that function. Replaced the logic I had written, but some preprocessing was still required due to how the byline string can be formatted.

Show outdated Hide outdated Sveriges radio.js
// The title from the meta is in the format:
// Australierna säger ja till samkönade äktenskap - Nyheter (Ekot)
var titleString =
ZU.xpath(doc, '//meta[@name="twitter:title"]')[0]

This comment has been minimized.

@adam3smith

adam3smith Feb 27, 2018

Collaborator

use attr and CSS selector here instead for simplicity

@adam3smith

adam3smith Feb 27, 2018

Collaborator

use attr and CSS selector here instead for simplicity

Show outdated Hide outdated Sveriges radio.js
item.section = titleParts[1];
var dateString =
ZU.xpath(doc, '//meta[@name="displaydate"]')[0]

This comment has been minimized.

@adam3smith

adam3smith Feb 27, 2018

Collaborator

same here

@adam3smith

adam3smith Feb 27, 2018

Collaborator

same here

Show outdated Hide outdated Sveriges radio.js
var dateString =
ZU.xpath(doc, '//meta[@name="displaydate"]')[0]
.getAttribute("content");
item.date = dateString.replace(/(....)(..)(..)/, "$1-$2-$3");

This comment has been minimized.

@adam3smith

adam3smith Feb 27, 2018

Collaborator

(\d{4})(\d{2})(\d{2}) is more precise and more readable

@adam3smith

adam3smith Feb 27, 2018

Collaborator

(\d{4})(\d{2})(\d{2}) is more precise and more readable

Show outdated Hide outdated Sveriges radio.js
translator.getTranslatorObject(function(trans) {
trans.itemType = "newspaperArticle";
// TODO map additional meta tags here, or delete completely

This comment has been minimized.

@adam3smith

adam3smith Feb 27, 2018

Collaborator

remove all TODOs

@adam3smith

adam3smith Feb 27, 2018

Collaborator

remove all TODOs

Show outdated Hide outdated Sveriges radio.js
"title": "Australierna säger ja till samkönade äktenskap",
"creators": [
{
"firstName": "Ekot",

This comment has been minimized.

@adam3smith

adam3smith Feb 27, 2018

Collaborator

using Zotero's built in function will solve this, but the right way to handle single-name and institutional authors is to use lastName only and set fieldMode: true

@adam3smith

adam3smith Feb 27, 2018

Collaborator

using Zotero's built in function will solve this, but the right way to handle single-name and institutional authors is to use lastName only and set fieldMode: true

Show outdated Hide outdated Sveriges radio.js
"title": "Snapshot"
}
],
"tags": [

This comment has been minimized.

@adam3smith

adam3smith Feb 27, 2018

Collaborator

These tags look generic and not helpful. I'd clean them out.

@adam3smith

adam3smith Feb 27, 2018

Collaborator

These tags look generic and not helpful. I'd clean them out.

This comment has been minimized.

@sebastian-berlin-wmse

sebastian-berlin-wmse Feb 28, 2018

Contributor

I wasn't sure what tags did, so I kept them as they were. As you say, at least some of them are quite general and don't seem to relate to the article. Happy to get rid of them.

@sebastian-berlin-wmse

sebastian-berlin-wmse Feb 28, 2018

Contributor

I wasn't sure what tags did, so I kept them as they were. As you say, at least some of them are quite general and don't seem to relate to the article. Happy to get rid of them.

@sebastian-berlin-wmse

This comment has been minimized.

Show comment
Hide comment
@sebastian-berlin-wmse

sebastian-berlin-wmse Feb 28, 2018

Contributor

Thanks for the feedback, @adam3smith. I've fixed the issues you mentioned. Let me know if there's anything else.

Contributor

sebastian-berlin-wmse commented Feb 28, 2018

Thanks for the feedback, @adam3smith. I've fixed the issues you mentioned. Let me know if there's anything else.

Add translator for sverigesradio.se
This is the website for the Swedish public service radio.

@adam3smith adam3smith merged commit 11e78ea into zotero:master Mar 4, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@adam3smith

This comment has been minimized.

Show comment
Hide comment
@adam3smith

adam3smith Mar 4, 2018

Collaborator

Great, thanks!

Collaborator

adam3smith commented Mar 4, 2018

Great, thanks!

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