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

Add Tesis Doctorals en Xarxa (TDX) #1213

Merged
merged 6 commits into from May 2, 2017

Conversation

Projects
None yet
4 participants
@CSUC
Copy link
Contributor

CSUC commented Jan 4, 2017

No description provided.

@CSUC

This comment has been minimized.

Copy link
Contributor Author

CSUC commented Jan 19, 2017

Any comment? we would want to use it as soon as possible, could anyone please check it?

Thank you very much

@zuphilip
Copy link
Collaborator

zuphilip left a comment

I just looked at the first lines in details and glanced over the following code. Thus, there are just some comments from the first lines.

But it seems that you a lot of meta tags and just one or to other elements. Therefore I guess that the current metadata with the Embedded Metadata translator is already pretty good. No? Shouldn't we then better first start with calling EM translator and adjust the remaining things afterwards? Here is my template for such a case https://github.com/zuphilip/translators/wiki/Calling-metadata-translator

"translatorID": "dd2d7c63-0c4b-4da4-b1d8-e72c7445c1e0",
"label": "Tesis Doctorals en Xarxa",
"creator": "CSUC",
"target": "^https?://(www\.)?(tdx\.cat|tesisenred\.net)",

This comment has been minimized.

Copy link
@zuphilip

zuphilip Jan 19, 2017

Collaborator

The regexp in JSON needs to escape the period . with double backslash, i.e. use

"target": "^https?://(www\\.)?(tdx\\.cat|tesisenred\\.net)",
"priority": 100,
"inRepository": true,
"translatorType": 4,
"browserSupport": "gcsb",

This comment has been minimized.

Copy link
@zuphilip

zuphilip Jan 19, 2017

Collaborator

By default we add here all, i.e. "gcsibv"

"lastUpdated": "2016-09-22 15:33:33"
}


This comment has been minimized.

Copy link
@zuphilip

zuphilip Jan 19, 2017

Collaborator

All translators need a license block (AGPL), see the example here https://github.com/zuphilip/translators/wiki/Common-code-blocks-for-translators#licence-block

This comment has been minimized.

Copy link
@adam3smith

adam3smith Jan 19, 2017

Collaborator

@zuphilip which reminds me -- we should write a style guide for translator coding.

This comment has been minimized.

Copy link
@zuphilip

zuphilip Jan 19, 2017

Collaborator

Yeah, I think what is currently missing is a README.md and a file CONTRIBUTING (which we can link from the README.md). A style guide could be of this? We can also discuss this further in a new issue, if you would like.

@CSUC

This comment has been minimized.

Copy link
Contributor Author

CSUC commented Jan 20, 2017

Hi! Thank you! I've done some changes adding the right escape on regex and adding the missed license block.
Is it needed to add the getSearchResults function?

@zuphilip

This comment has been minimized.

Copy link
Collaborator

zuphilip commented Jan 20, 2017

These things looks good now. The function getSearchResults is not strictly needed, but it is often used in translators as a general pattern.

Let me repeat my comment and questions from above for the rest:

But it seems that you a lot of meta tags and just one or to other elements. Therefore I guess that the current metadata with the Embedded Metadata translator is already pretty good. No? Shouldn't we then better first start with calling EM translator and adjust the remaining things afterwards? Here is my template for such a case https://github.com/zuphilip/translators/wiki/Calling-metadata-translator

@CSUC

This comment has been minimized.

Copy link
Contributor Author

CSUC commented Jan 20, 2017

Hi @zuphilip, thank you for your help! I've changed the code following the Translator template, I hope I didn't forgot anything else.

translator.setDocument(doc);
translator.setHandler('itemDone', function (obj, item) {
//title
var titles = ZU.xpath(doc, '//meta[@name="citation_title"]');

This comment has been minimized.

Copy link
@zuphilip

zuphilip Jan 20, 2017

Collaborator

I am quite sure that all these metadata fields are already capture, when you arise her. Can you comment out everything in this function expect the item.complete();? What is the result then and which parts are missing then? I would expect that only very few lines will be needed in the end here in this setHandler function.

This comment has been minimized.

Copy link
@adam3smith

adam3smith Jan 22, 2017

Collaborator

yes, please do that. In addition, for the fields you do need to add or change, using item.field = ZU.xpathText(doc, xpath) is much simpler and equivalent to what you're doing with ZU.xpath.

@adam3smith
Copy link
Collaborator

adam3smith left a comment

I'd actually want to insist on using a getSearchResults function or at least some other more reliable detection of multiples. For example, this translator will detect http://www.tesisenred.net/contacte as multiple in its current version.

translator.setDocument(doc);
translator.setHandler('itemDone', function (obj, item) {
//title
var titles = ZU.xpath(doc, '//meta[@name="citation_title"]');

This comment has been minimized.

Copy link
@adam3smith

adam3smith Jan 22, 2017

Collaborator

yes, please do that. In addition, for the fields you do need to add or change, using item.field = ZU.xpathText(doc, xpath) is much simpler and equivalent to what you're doing with ZU.xpath.

function detectWeb(doc, url) {
var type = ZU.xpath(doc, '//meta[@name="DC.type"]/@content');
if (type.length>0) {
if (mappingTable[type[0].textContent]) {

This comment has been minimized.

Copy link
@adam3smith

adam3smith Jan 22, 2017

Collaborator

while this is generally a good approach, it seems silly to do this as long as the whole excercise will just lead to a thesis item type either way. If you think the site will add other items in the feature, I'd be OK with you just commenting this (and the then unused mapping function) out. If this will likely remain a thesis repository, just return thesis.

@CSUC

This comment has been minimized.

Copy link
Contributor Author

CSUC commented Jan 30, 2017

Done more changes following all the tips

@zuphilip

This comment has been minimized.

Copy link
Collaborator

zuphilip commented Jan 31, 2017

This looks pretty good now. Thank you! I haven't looked at the details, but I am sure that @adam3smith will do that.

@adam3smith
Copy link
Collaborator

adam3smith left a comment

OK, we should be almost there. Some nits remaining, but should be quick fixes.

}

//deleting all unwanted tags
item.reportType=item.letterType=item.manuscriptType=item.mapType=item.websiteType=item.presentationType=item.postType=item.audioFileType=null

This comment has been minimized.

Copy link
@adam3smith

adam3smith Feb 8, 2017

Collaborator

no need for this. Zotero discards them.

item.rights = [];
for (var t=0; t<rights.length; t++) {
if (mappingRights[rights[t].content])
item.rights.push(mappingRights[rights[t].content]);

This comment has been minimized.

Copy link
@adam3smith

adam3smith Feb 8, 2017

Collaborator

you end up with an array here. item.rights expects a string. You'd want to run someting like
item.rights = item.rights.join(", ") or some other operation that converts this to a string.
But I'm also wondering: are there items with multiple rights metatags?

This comment has been minimized.

Copy link
@CSUC

CSUC Feb 17, 2017

Author Contributor

Yes, they have multiple rights metatags, I will include this join sentence.


/** BEGIN TEST CASES **/
/** BEGIN TEST CASES **/
var testCases = [

This comment has been minimized.

Copy link
@adam3smith

adam3smith Feb 8, 2017

Collaborator

Are these tests created with Scaffold? The most recent version shouldn't be including all those extraneous variables in fields. It would also get spacing right and not insert spaces at the beginning of some lines.
Please fix.

This comment has been minimized.

Copy link
@CSUC

CSUC Feb 17, 2017

Author Contributor

I did it with Scaffold,, I have deleted all spaces at the beggining of some lines as you said

CSUC and others added some commits Feb 17, 2017

@CSUC

This comment has been minimized.

Copy link
Contributor Author

CSUC commented Apr 4, 2017

Hi!
Is still not valid? Is there any change pending to be done?

Thank you.

@toniher

This comment has been minimized.

Copy link

toniher commented May 2, 2017

Hi! anything that can be done to help on this? After merged and once included in Citoid, this translator will be very useful in some Wikipedias. Thanks!

@adam3smith adam3smith merged commit 29db7c9 into zotero:master May 2, 2017

1 check passed

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

This comment has been minimized.

Copy link
Collaborator

adam3smith commented May 2, 2017

Sorry for letting this linger & thanks for contributing.

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

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

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.