Perfect your code
With built-in code review tools, GitHub makes it easy to raise the quality bar before you ship. Join the 40 million developers who've merged over 200 million pull requests.
Sign up for free See pricing for teams and enterprisesAdd translator for new NASA ADS #2055
Conversation
thostetler
force-pushed the
thostetler:add-nasa-ads-2-translator
branch
from
c8ab752
to
7351483
thostetler
force-pushed the
thostetler:add-nasa-ads-2-translator
branch
from
d9794b2
to
1be1530
| if (url.includes("/search/")) { | ||
| return "multiple"; | ||
| } | ||
| else if (url.includes("/abs/")) { |
This comment has been minimized.
This comment has been minimized.
adam3smith
Nov 13, 2019
Collaborator
How hard would it be to detect some of the other content types here? It's not super critical to detect the right type, but if e.g. matching by the id would get us mostly there, it's worth the effort (e.g. it looks like all PhD Thesis have PhDT in the ID etc.
This comment has been minimized.
This comment has been minimized.
thostetler
Nov 20, 2019
Author
Contributor
Yep, I can make that change. I know for sure we can extract PhDT and MsT
This comment has been minimized.
This comment has been minimized.
|
Is the old translator for NASA ADS then still working? Or should we rather replace the old one with the code here? I don't like the label here "NASA ADS 2". |
This comment has been minimized.
This comment has been minimized.
|
@zuphilip The old system has been shutdown now, so the other translator won't work. |
This comment has been minimized.
This comment has been minimized.
|
OK, if this is a replacement let's do this as a modification of the existing file with the same translatorID and title. (You can make that change in this PR without opening a new one.) |
This comment has been minimized.
This comment has been minimized.
|
@dstillman sure I can make that change |
thostetler
force-pushed the
thostetler:add-nasa-ads-2-translator
branch
from
21702af
to
5a07668
|
Great, thanks. A couple of remaining nits, after that this is ready from my end |
| @@ -1,22 +1,22 @@ | |||
| { | |||
| "translatorID": "7987b420-e8cb-4bea-8ef7-61c2377cd686", | |||
| "label": "NASA ADS", | |||
| "creator": "Philipp Zumstein, Asa Kusuma and Ramesh Srigiriraju", | |||
| "target": "^https?://(ukads|cdsads|ads|adsabs|esoads|adswww|www\\.ads)\\.(inasan|iucaa\\.ernet|nottingham\\.ac|harvard|eso|u-strasbg|nao\\.ac|astro\\.puc|bao\\.ac|on|kasi\\.re|grangenet|lipi\\.go|mao\\.kiev)\\.(edu|org|net|fr|jp|cl|id|uk|cn|ua|in|ru|br|kr)/(cgi-bin|abs)/", | |||
| "creator": "Philipp Zumstein, Asa Kusuma, Ramesh Srigiriraju and Tim Hostetler", | |||
This comment has been minimized.
This comment has been minimized.
adam3smith
Nov 21, 2019
Collaborator
Since this is a rewrite, you should be the only creator I think
| } | ||
|
|
||
| /* | ||
| ***** BEGIN LICENSE BLOCK ***** | ||
| Copyright © 2018 Philipp Zumstein | ||
| Copyright © 2019 Philipp Zumstein |
This comment has been minimized.
This comment has been minimized.
| translator.translate(); | ||
| }); | ||
| if (bibstem.startsWith("MsT")) { | ||
| return "Masters thesis"; |
This comment has been minimized.
This comment has been minimized.
adam3smith
Nov 21, 2019
Collaborator
Only thesis is a valid Zotero item type. You could see if you can get the Master vs. Ph.D. thesis into the type field in Zotero
This comment has been minimized.
This comment has been minimized.
thostetler
Nov 21, 2019
Author
Contributor
So I have a question on this, I updated the itemType returned here to be thesis.
const bibstem = extractId(url).slice(4);
if (bibstem.startsWith("MsT") || bibstem.startsWith("PhDT")) {
return "thesis";
}
return "journalArticle";
I can get the correct output: detectWeb returned type "thesis" when running detectWeb from the editor. But when I run the doWeb function I always get:
Returned item:
{
"itemType": "journalArticle"
...
}
Is this being set by the RIS importer? or is there something that I'm forgetting to set prior to passing that text?
This comment has been minimized.
This comment has been minimized.
adam3smith
Nov 21, 2019
Collaborator
Yes, I'm pretty sure that's the RIS importer. You have
TY - Thesis/Dissertation in the RIS, which isn't valid and Zotero uses journalArticle as a generic fallback for those cases. It should instead be
TY - THES It'd be best to change it in the RIS so that everyone benefits, but if that's impossible, you can overried the importers item type in the translator.
This comment has been minimized.
This comment has been minimized.
thostetler
Nov 25, 2019
Author
Contributor
@adam3smith So I talked with some people on the team about this, it seems like the THES is just an abbreviation and that the longer version is also valid (at least this is what I've heard) so I don't think it will be changing on our end.
I've updated the PR though to do it manually, hopefully I did it the right way.
Thanks!
| "url": "https://ui.adsabs.harvard.edu/abs/2019PhDT........69B/abstract", | ||
| "items": [ | ||
| { | ||
| "itemType": "Ph.D. thesis", |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Thank you for replacing the file now as the old system is down. I let the remaining review to @adam3smith . |
This comment has been minimized.
This comment has been minimized.
|
Great, thanks so much @thostetler |
thostetler commentedNov 11, 2019
New translator for the new NASA ADS site, using the same RIS export endpoint as the old translator.