Skip to content

/translators

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

Reviewers

@zuphilip zuphilip

@adam3smith adam3smith

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@pjalet @adam3smith @zuphilip
@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

@pjalet
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.
Loading status checks…
8e25dcc
@adam3smith
Fix spacing & typo
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
35feb04
@adam3smith
more spaces
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
a821891
@adam3smith

This comment has been minimized.

Sign in to view
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

@pjalet
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)
Loading status checks…
a37d3d2
@pjalet

This comment has been minimized.

Sign in to view
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.

Sign in to view
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

@zuphilip

zuphilip reviewed Oct 7, 2018
View changes

ESpacenet.js Outdated
@@ -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.

Sign in to view
Copy link
@zuphilip

zuphilip Oct 7, 2018

Collaborator

Add a space after the comma.
@zuphilip

zuphilip reviewed Oct 7, 2018
View changes

ESpacenet.js Outdated
@@ -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.

Sign in to view
Copy link
@zuphilip

zuphilip Oct 7, 2018

Collaborator

There is not always such a "Global Dossier" button, see e.g. https://worldwide.espacenet.com/data/publicationDetails/biblio?locale=en_EP&II=9&FT=D&CC=AU&DB=EPODOC&NR=2814389A&date=19890601&ND=3&KC=A&adjacent=true
@zuphilip
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.
Loading status checks…
95c23dd
@zuphilip

This comment has been minimized.

Sign in to view
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

1 check passed

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

This comment has been minimized.

Sign in to view
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.