Fetcher OttoBib #5125

allisonsampaio · 2019-07-14T14:06:34Z

I developed the fetcher to use ottobib, then after it try the ebook.de and chimbori.com is made the requisition for ottobib. #2581

Siedlerchr · 2019-07-14T16:36:31Z

look at the WebFetchersTest class under JabRef/src/test/java/org/jabref/logic/importer/WebFetchersTest.java
You need to remove your new fetcher from the list of expected fetchers, that should resolve the erros

allisonsampaio · 2019-07-14T21:51:58Z

look at the WebFetchersTest class under JabRef/src/test/java/org/jabref/logic/importer/WebFetchersTest.java
You need to remove your new fetcher from the list of expected fetchers, that should resolve the erros

Hi @Siedlerchr ! Thanks for answering me. I think the problem with WebFetchersTest has been fixed.

Siedlerchr

Thanks for your contribution , looks good to me know!
Just remove the unused method and add a change log entry, e.g. We improved fetching bibtex data from an ISBN

Siedlerchr · 2019-07-15T06:04:25Z

src/main/java/org/jabref/logic/importer/fetcher/IsbnViaOttoBibFetcher.java

+ }
+
+ @Override
+ public void doPostCleanup(BibEntry entry) {


You can remove that method if it's not used

I removed the method I was not using. I'll do the unit test soon.

Siedlerchr

I forgot, please also add a unit test for the new fetcher

Siedlerchr

Thanks for your contribution! Now it's good!
If a second JabRef dev gives his okay, we can merge (Ideally a PR should be always reviewed by at least 2 JabRef devs)

LinusDietz

I have some rather nitpicky, but hopefully useful remarks. Functionality looks good, though. Can be merged after addressing the comments.

LinusDietz · 2019-07-16T12:47:56Z

src/main/java/org/jabref/logic/importer/fetcher/IsbnViaOttoBibFetcher.java

+
+ try {
+ postResponse = Unirest.post(BASE_URL)
+ .asString();


Please put this into the line before

LinusDietz · 2019-07-16T12:49:36Z

src/main/java/org/jabref/logic/importer/fetcher/IsbnViaOttoBibFetcher.java

+ * @return null, because the identifier is passed using form data. This method is not used.
+ */
+ @Override
+ public URL getURLForID(String identifier) throws URISyntaxException, MalformedURLException, FetcherException {


Please remove these exceptions. They can never be thrown.

@LinusDietz You are right that those exceptions can never be thrown, but the Exception declaration is coming from the interface IdBasedParserFetcher

Why is it not possible to implement the ottobib fetcher by properly overwriting getURLForID as e.g here:

jabref/src/main/java/org/jabref/logic/importer/fetcher/IsbnViaEbookDeFetcher.java

Lines 32 to 38 in e89d603

@Override

public URL getURLForID(String identifier) throws URISyntaxException, MalformedURLException, FetcherException {

this.ensureThatIsbnIsValid(identifier);

URIBuilder uriBuilder = new URIBuilder(BASE_URL);

uriBuilder.addParameter("isbn", identifier);

return uriBuilder.build().toURL();

}

I don't really see why the performSearchById has to be overwritten (which is more work as you have to manually post the request and parse the response).

LinusDietz · 2019-07-16T12:52:20Z

src/main/java/org/jabref/logic/importer/fetcher/IsbnViaOttoBibFetcher.java

+
+ HttpResponse<String> postResponse;
+
+ String BASE_URL = "https://www.ottobib.com/isbn/" + identifier + "/bibtex";


I would propose to make BASE_URL final.

Also, I would prefer a String.format(): String.format("https://www.ottobib.com/isbn/%s/bibtex", identifier);

LinusDietz · 2019-07-16T12:59:21Z

src/test/java/org/jabref/logic/importer/fetcher/IsbnViaOttoBibFetcherTest.java

+public class IsbnViaOttoBibFetcherTest extends AbstractIsbnFetcherTest {
+
+ @BeforeEach
+ public void setUp() {


I would suggest to convert this into a static method createSampleBibEntry(). See Java by Comparison, "Favor Standalone Tests".

LinusDietz · 2019-07-16T13:01:14Z

src/test/java/org/jabref/logic/importer/fetcher/IsbnViaOttoBibFetcherTest.java

+ bibEntry.setField("isbn", "978-2-8195-02746");
+ bibEntry.setField("url", "https://www.ottobib.com/isbn/9782819502746/bibtex");
+
+ fetcher = new IsbnViaOttoBibFetcher(mock(ImportFormatPreferences.class, Answers.RETURNS_DEEP_STUBS));


Move fetcher into the respective tests to adhere to the 'given-then-when' structure of tests.

Just out of interest to learn something, but doesn't this give (needless ?) code duplication especially since all tests are of the form "for-input-x-verify-output-y". If the state of the fetcher needs to be varied then a test of the form

@Test public void test() throws FetcherException { fetcher.setSomething(value); Entry fetchedEntry = fetcher.fetch(input); assertEquals(expected, fetchedEntry); }

still perfectly adheres to the usual test pattern.

Fetcher does not return bibtex data in plain text, instead it's part of an html text area Fix ISBN tests Update user agent Follow up from #5125

Siedlerchr · 2019-08-18T16:25:55Z

Superseeded by #5196

a20406d Added name of the editors of a given edition (#5140) 9881fc5 Ping on push, not PR, document role of dist-updater (#5137) 04668cc Create nouvelles-perspectives-en-sciences-sociales.csl (#5063) 1d94e21 Update bursa-uludag-universitesi-saglik-bilimleri-enstitusu.csl (#5047) 84f3893 Add Harvard style for Metropolia University of Applied Sciences (#5086) 8e43e79 Create opto-electronic-advances.csl (#5135) 36e4fba Update society-for-american-archaeology.csl (#5124) 69ca360 St. Paul Canon Law new style (#5138) b490ab0 Update and rename st-paul-university-faculty-of-canon-law.csl to saint-paul-university-faculty-of-canon-law.csl b498116 There is no en-CA locale 3c35f28 Metadata 7059cca Create tu-dortmund-agvm.csl (#5088) c321c98 Create new Citation type (#5093) a7edc8d Update international-organization.csl (#5103) 3d1a052 The AWS load balancer is messing things up (#5133) ca3839b Fix sort by a single macro (#5136) 5d1a7e8 Update chungara-revista-de-antropologia-chilena.csl (#5123) cd75d5d ping distribution-updater (#5132) dcf473a Update wirtschaftsuniversitat-wien-health-care-management.csl (#5125) a87085e Fix Harvard Praxisforschung Editors (#5130) d4176ca Switch automated tests to Github Actions (#5111) 726d0d8 Radiology, MPP, CORR -- small fixes: https://forums.zotero.org/discussion/85883/doi-radiology#latest https://forums.zotero.org/discussion/51058/style-request-molecular-plant-pathology#latest https://forums.zotero.org/discussion/85678/citing-style-clinical-orthopaedics-and-related-research#latest e23db68 Update to la-trobe-university-harvard style (#5119) c54b278 Create wirtschaftsuniversitat-wien-health-care-management.csl (#5110) 62fb019 Create austral-entomology.csl (#5118) afa328c Update iso690-author-date-en.csl (#5113) 5468dce Update iso690-author-date-fr-no-abstract.csl (#5112) 98af86c Update iso690-numeric-fr.csl (#5115) 09f84c4 Update iso690-author-date-fr.csl (#5114) 178a9e4 Fix current biology to superscript 1fa5ce7 Create droit-belge-centre-de-droit-prive-ulb.csl (#5107) 3a6a4bc Fix file modes (#5106) 48f50e5 Create chungara-revista-de-antropologia-chilena.csl (#5096) 1e848f8 Create the-journal-of-the-indian-law-institute.csl (#5100) 856524c Create molecular-biology.csl (#5101) eeebbf4 Create harvard-harper-adams-university.csl (#5104) d90993d Fix tests d4037bf WIP: St Paul Canon Law style git-subtree-dir: src/main/resources/csl-styles git-subtree-split: a20406d

fetcher ottobib

56ca5c9

allisonsampaio added 4 commits July 14, 2019 17:45

removed new fetcher ottobib from the list of expected fetchers

ce4ae96

changed the order of the importer

46b1bfc

changed the order of the importer

7c90f7a

import

477432a

allisonsampaio mentioned this pull request Jul 14, 2019

fetcher Ottobib #5122

Closed

Siedlerchr approved these changes Jul 15, 2019

View reviewed changes

Removed unused method

10f1450

Siedlerchr requested changes Jul 15, 2019

View reviewed changes

allisonsampaio added 4 commits July 15, 2019 08:29

removed method call

72776cc

added a unit test

77606fa

Merge branch 'master' of https://github.com/sampaiox/jabref

992f63f

added a unit test

236d622

Siedlerchr approved these changes Jul 15, 2019

View reviewed changes

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jul 15, 2019

LinusDietz requested changes Jul 16, 2019

View reviewed changes

LinusDietz added fetcher and removed status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Jul 16, 2019

Siedlerchr added a commit that referenced this pull request Aug 18, 2019

Implement ottobib fetcher

526fcf6

Fetcher does not return bibtex data in plain text, instead it's part of an html text area Fix ISBN tests Update user agent Follow up from #5125

Siedlerchr mentioned this pull request Aug 18, 2019

Implement ottobib fetcher #5196

Merged

6 tasks

Siedlerchr closed this Aug 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fetcher OttoBib #5125

Fetcher OttoBib #5125

allisonsampaio commented Jul 14, 2019 •

edited

Loading

Siedlerchr commented Jul 14, 2019

allisonsampaio commented Jul 14, 2019

Siedlerchr left a comment

Siedlerchr Jul 15, 2019

allisonsampaio Jul 15, 2019

Siedlerchr left a comment

Siedlerchr left a comment

LinusDietz left a comment

LinusDietz Jul 16, 2019

LinusDietz Jul 16, 2019

Siedlerchr Jul 16, 2019

tobiasdiez Jul 16, 2019

LinusDietz Jul 16, 2019

LinusDietz Jul 16, 2019

LinusDietz Jul 16, 2019

tobiasdiez Jul 16, 2019

Siedlerchr commented Aug 18, 2019

	@Override
	public URL getURLForID(String identifier) throws URISyntaxException, MalformedURLException, FetcherException {
	this.ensureThatIsbnIsValid(identifier);
	URIBuilder uriBuilder = new URIBuilder(BASE_URL);
	uriBuilder.addParameter("isbn", identifier);
	return uriBuilder.build().toURL();
	}


		HttpResponse<String> postResponse;

		String BASE_URL = "https://www.ottobib.com/isbn/" + identifier + "/bibtex";

Fetcher OttoBib #5125

Fetcher OttoBib #5125

Conversation

allisonsampaio commented Jul 14, 2019 • edited Loading

Siedlerchr commented Jul 14, 2019

allisonsampaio commented Jul 14, 2019

Siedlerchr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr left a comment

Choose a reason for hiding this comment

Siedlerchr left a comment

Choose a reason for hiding this comment

LinusDietz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr commented Aug 18, 2019

allisonsampaio commented Jul 14, 2019 •

edited

Loading