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

Bug fixes (especially issue with locale identification) and minor improvements #1748

Merged
merged 5 commits into from Nov 12, 2018

Conversation

Projects
None yet
3 participants
@pjalet
Copy link
Contributor

pjalet commented Sep 21, 2018

Locale identification failed because the URL may end with an # -> fixed with new regex on line 93.
Using secondaryInventors and secondaryApplicants led to duplicates -> fixed by using inventors and applicants instead (l. 156, 165)
Names from inventors and applicants include country code in brackets -> fixed with modified regex on line 129.
Case correction did not work when some names where upper case and some not -> fixde by eliminating test on line 131 and going first to lower case on line 132.
Catching the application number through default case led to inclusion of parasitic text ("global dossier") -> fixed with specific case and XPath on line 190
Current patent number is directly accessible through navigation block on left of the page, so that text cleanup is not necessary -> new assignement on line 204.

pjalet and others added some commits Sep 21, 2018

Bug fix (issue with locale identification) and minor improvements
Locale identification failed because the URL may end with an # -> fixed with new regex on line 93.
Using secondaryInventors  and secondaryApplicants led to duplicates -> fixed by using inventors and applicants instead (l. 156, 165)
Names from inventors and applicants include country code in brackets -> fixed with modified regex on line 129.
Case correction did not work when some names where upper case and some not -> fixde by eliminating test on line 131 and going first to lower case on line 132.
Catching the application number through default case led to inclusion of parasitic text ("global dossier") -> fixed with specific case and XPath on line 190
Current patent number is directly accessible through navigation block on left of the page, so that text cleanup is not necessary -> new assignement on line 204.
@adam3smith

This comment has been minimized.

Copy link
Collaborator

adam3smith commented Sep 28, 2018

Thanks! The URL fix is definitely fine. For the other issues -- did you run the tests? I would assume they'd cause some updated test data?

Also, please remove the change notes in the code -- they make no sense without the previous code.

@adam3smith adam3smith added the waiting label Sep 28, 2018

Cleanup + update to test cases
Eliminated references to previous code in comments.
Direct assignment of newItem.patentNumber without intermediary variable (line 196).
Update to test cases (lines 333, 366)
@pjalet

This comment has been minimized.

Copy link
Contributor Author

pjalet commented Oct 2, 2018

Good point regarding the tests. The changes indeed have an impact on the expected patent number; this is changed in the latest commit. However the tests consistently return an error ("TranslatorTester: ESpacenet Test 1: failed (Detection failed)") in Scaffold but on this code and the original one in zotero:master.
Change notes in the code are removed.

@zuphilip

This comment has been minimized.

Copy link
Collaborator

zuphilip commented Oct 7, 2018

However the tests consistently return an error ("TranslatorTester: ESpacenet Test 1: failed (Detection failed)") in Scaffold but on this code and the original one in zotero:master.

I guess this is because the detection uses Z.monitorDOMChanges function which the automatic testing is IMO ignoring. You can temporarily output the expected type in detectWeb always, and then run the tests again. (Maybe even for the test cases save the types hard coded, i.e. if url = ... then type = ...)

@zuphilip zuphilip removed the waiting label Oct 7, 2018

@@ -187,6 +181,9 @@ function scrape(doc) {
case "Page bookmark":
applyValue(newItem, label, value.firstElementChild.href)
break;
case "Application number:":
applyValue(newItem, label, ZU.xpathText(value,'./node()[following-sibling::a]'));

This comment has been minimized.

Copy link
@zuphilip

zuphilip Oct 7, 2018

Collaborator

Add a space after the comma.

@@ -187,6 +181,9 @@ function scrape(doc) {
case "Page bookmark":
applyValue(newItem, label, value.firstElementChild.href)
break;
case "Application number:":
applyValue(newItem, label, ZU.xpathText(value,'./node()[following-sibling::a]'));

This comment has been minimized.

Some nits, new test urls, update test cases
For some reason it works when we use urls with additional
"/data" part, see updated test cases.
@zuphilip

This comment has been minimized.

Copy link
Collaborator

zuphilip commented Oct 7, 2018

Okay, I did some small updates and found test cases which seem to work within Scaffold. Looks fine for me now. @adam3smith Can you do the final review?

@zuphilip zuphilip assigned adam3smith and unassigned adam3smith Oct 9, 2018

@zuphilip zuphilip requested a review from adam3smith Oct 9, 2018

@adam3smith adam3smith merged commit 452df57 into zotero:master Nov 12, 2018

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 Nov 12, 2018

Thanks and sorry for the delay; I missed the review request.

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.