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

PubMedLabs update to PubMed.js #1801

Merged
merged 17 commits into from Feb 4, 2019

Conversation

Projects
None yet
4 participants
@akinhwan
Copy link
Contributor

akinhwan commented Dec 19, 2018

updated getUID and getSearchResults functions, added a testCase for PubMedLabs

  • for the function getUID(), should I have logic for when user is on the "search results page" where there might be multiple UIDs? Or is this function purposefully meant for single UID at a time?
Show resolved Hide resolved PubMed.js Outdated
Show resolved Hide resolved PubMed.js Outdated
Show resolved Hide resolved PubMed.js Outdated
Show resolved Hide resolved PubMed.js Outdated
@akinhwan

This comment has been minimized.

Copy link
Contributor Author

akinhwan commented Dec 21, 2018

@zuphilip Hi Phillip, thanks for writing the PubMed.js. I'm trying to fix some user errors noticed on the new Pub Med Labs site. Namely, abstract pages are not being recognizes as "journalArticle" types. Are the commented out code in the doWeb function still needed?

@adam3smith

This comment has been minimized.

Copy link
Collaborator

adam3smith commented Dec 21, 2018

Are the commented out code in the doWeb function still needed?

Almost certainly not, no. These have been there for a couple of years and no one has missed them. Feel free to remove during re-write.

Kim

@akinhwan akinhwan changed the title [WIP] PubMedLabs update to PubMed.js PubMedLabs update to PubMed.js Dec 24, 2018

@akinhwan

This comment has been minimized.

Copy link
Contributor Author

akinhwan commented Dec 26, 2018

All tests are passing in Scaffold!

zotero_scaffold_tests

@akinhwan

This comment has been minimized.

Copy link
Contributor Author

akinhwan commented Dec 31, 2018

@adam3smith could you take a look at this PR when you get a chance? I made sure all tests are passing in Scaffold. Let me know if there is anything you'd like me to address! Thanks

akinhwan and others added some commits Jan 2, 2019

@CheViana

This comment has been minimized.

Copy link

CheViana commented Jan 2, 2019

Could somebody with write access to repo please merge this? @adam3smith @zuphilip

@adam3smith

This comment has been minimized.

Copy link
Collaborator

adam3smith commented Jan 3, 2019

We'll need to review before merging, but as we indicated at @akinhwan ' s post above, are aware of this and will do asap.

adam3smith added some commits Feb 2, 2019

@adam3smith

This comment has been minimized.

Copy link
Collaborator

adam3smith commented Feb 3, 2019

@zuphilip -- do you have time to give this a quick look. I ran all automated tests without problems.

Fix two other warnings of the linter
The escape function is deprecated and therefore
use decodeURIComponent instead.
@zuphilip
Copy link
Collaborator

zuphilip left a comment

This looks good. I have fixed two other warnings in the old code and added some comments. I have two more questions for you @adam3smith .

PubMed.js Outdated
if (!main) return;

var booksections = doc.getElementById('book-sections');
if (!main && !booksections) return;

This comment has been minimized.

@zuphilip

zuphilip Feb 3, 2019

Collaborator

I don't understand the logic here which you just changed recently. It is possible that main does not exist but booksections does, then this would continue (not both conditions fulfilled) and the next line would fail. I.e. main has to exist for the next line, but then we can change back to the original condition, because this will not change anything else. 🤔

This comment has been minimized.

@adam3smith

adam3smith Feb 4, 2019

Collaborator

Good catch. I'm not sure there are cases where just booksections appears, but in any case we're not using them, so I've removed booksections here entirely and make sure we don't expect it in detectWeb.

It might be that there are book sections without UID and for those this translator won't detect (or import) now -- if we find one in the future, we can simply add logic in this function.

Given that the parseFromItemprop function only covers books, I don't believe anyone has every come across an example.

PubMed.js Outdated
title: ZU.trimInternal(title),
checked: checkbox && checkbox.checked
};
if (results.length > 0){

This comment has been minimized.

@zuphilip

zuphilip Feb 3, 2019

Collaborator

Isn't it enough to either do the test on line 146 and fail when length is zero or do the condition here such that the following lines are only called when the length is positive?

@zuphilip zuphilip merged commit 5772369 into zotero:master Feb 4, 2019

1 check passed

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

This comment has been minimized.

Copy link
Collaborator

zuphilip commented Feb 4, 2019

🎉 Thank you very much @akinhwan @CheViana @adam3smith for working on this collaboratively! This is now squashed and merged and thereby will appear for the Zotero's users soon.

@akinhwan

This comment has been minimized.

Copy link
Contributor Author

akinhwan commented Feb 12, 2019

@zuphilip @adam3smith thank You so how would I know when it is live? Would it be in the next release? https://www.zotero.org/support/changelog Thanks :)

@adam3smith

This comment has been minimized.

Copy link
Collaborator

adam3smith commented Feb 12, 2019

Translators update daily, independent of releases, so this has been live for >a week.

GuyAglionby added a commit to GuyAglionby/translators that referenced this pull request Mar 30, 2019

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.