ISBN Fetcher using the new fetcher infrastructure #1654

zesaro · 2016-08-01T11:01:49Z

Implement an ISBN fetcher using the new fetcher infrastructure #1594

Change in CHANGELOG.md described
Tests created for changes
Manually tested changed features in running JabRef

tobiasdiez · 2016-08-01T14:30:32Z

src/test/java/net/sf/jabref/logic/importer/fetcher/IsbnFetcherTest.java

+    public void testFetcher10() throws FetcherException, IOException {
+        Optional<BibEntry> isbn = fetcher.performSearchById("0321356683");
+        Assert.assertNotNull(isbn);
+        try (InputStream bibStream = IsbnFetcherTest.class.getResourceAsStream("IsbnFetcherTest.bib")) {


Please create the BibEntry by hand instead of using the BIB file (see ArxivTests)

tobiasdiez · 2016-08-08T12:22:44Z

src/main/java/net/sf/jabref/gui/mergeentries/FetchAndMergeEntry.java

+                    try {
+                        fetchedEntry = new IsbnFetcher().performSearchById(fieldContent.get());
+                    } catch (FetcherException fe) {
+                        fe.printStackTrace();


see how exceptions are handled below for the ArXiv fetcher

tobiasdiez · 2016-08-08T12:40:09Z

I know, WIP but it look so good that I already reviewed it. After fixing my small remarks, this should be good to go

Siedlerchr · 2016-08-10T12:47:35Z

src/main/java/net/sf/jabref/gui/mergeentries/FetchAndMergeEntry.java

-                        fe.printStackTrace();
+                    } catch (FetcherException e) {
+                        panel.frame().setStatus(
+                                Localization.lang("Cannot get info based on given %0:_%1", type, fieldContent.get()));


Please log the exception, too, e.g. Logger.error/debug (just look at other code pieces 😄 )
And I think the underscore in the Localization is wrong, should be just a space.

koppor · 2016-08-19T17:02:19Z

src/main/java/net/sf/jabref/logic/importer/fetcher/IsbnFetcher.java

                throw new FetcherException("Bad URL when fetching ISBN info", e);
            }
        }
        return result;
    }
+
+    private Optional<BibEntry> postProcessEntry(BibEntry entry) {


I would return a BibEntry here as a BibEntry comes in and this is post processed. Always. The Optional is always present. Thus, the Optional conversion should be done by the caller.

stefan-kolb · 2016-08-23T08:12:21Z

@oscargus @tobiasdiez @koppor Are we ready to merge this?

oscargus · 2016-08-23T08:22:22Z

I would prefer that my comments are taken care of. On the other hand, it is quite quickly done, so not a show stopper if I have to do it myself after merging.

stefan-kolb · 2016-08-23T09:00:59Z

Let's wait for the comments to be implemented!

# Conflicts: # src/main/java/net/sf/jabref/gui/importer/fetcher/EntryFetchers.java # src/main/java/net/sf/jabref/gui/importer/fetcher/GeneralFetcher.java # src/main/java/net/sf/jabref/gui/importer/fetcher/ISBNtoBibTeXFetcher.java

…tcher # Conflicts: # src/main/java/net/sf/jabref/gui/importer/fetcher/EntryFetchers.java # src/main/java/net/sf/jabref/logic/importer/fetcher/IsbnFetcher.java

zesaro · 2016-08-25T15:54:33Z

Are the changes as expected?

stefan-kolb · 2016-08-25T16:12:41Z

@oscargus

oscargus · 2016-08-25T16:21:54Z

src/main/java/net/sf/jabref/logic/importer/fetcher/IsbnFetcher.java

+    private static final String URL_PATTERN = "http://www.ebook.de/de/tools/isbn2bibtex?";
+    private ImportFormatPreferences prefs;
+
+    public IsbnFetcher(){


I'd prefer that the constructor gets the ImportFormatPreferences object here (and, hence, move the ImportFormatPreferences.fromPreferences(Globals.prefs)
to EntryFetchers as

entryFetchers.add(new IdBasedEntryFetcher(new IsbnFetcher(ImportFormatPreferences.fromPreferences(Globals.prefs))));

In that way there won't be any import of Globals in logic. But a minor thing at the moment. Still things to fix which are much harder compared to this before being able to enforce it through tests.

Now it is: Globals.prefs.getImportFormatPreferences ()

oscargus · 2016-08-25T16:24:23Z

It is OK to merge for me. I commented on what I really would have liked to see, but it is easy to change later and the change provided makes it even easier. The location of IdBasedEntryFetcher is perfect! 👍

# Conflicts: # src/main/java/net/sf/jabref/gui/importer/fetcher/ISBNtoBibTeXFetcher.java

oscargus · 2016-08-29T09:38:32Z

LGTM! 👍 (The copyright comments should be removed as we changed license, but, again, minor detail and nothing that would stop me from merging.)

stefan-kolb · 2016-08-29T09:45:20Z

src/main/java/net/sf/jabref/gui/importer/fetcher/IdBasedEntryFetcher.java

+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *


Please remove the header.

* create test * make fetcher usable and organise imports * fix codacy * remove old isbn fetcher * ignore tests needing needing online connection * edit test and use wrapper for fetcher * add one more test to isbnfetcher * include feedback (1/2) * include feedback (2/2) * add logger to FetchAndMergeEntry * remove obsolete keys * resolve conflicts * include feedback and cleanup * include feedback (use Unirest and add more invalid tests) * include feedback * refactor globals and fix logger * refactor globals in isbnfetcher * resolve merge issues * remove copyright

koppor · 2016-09-12T21:01:09Z

Is it possible to update the fetcher to fix koppor#125? I think, just the normalization functionality has to be called. Maybe a cleanup job? Maybe after #1929 is merged to use the postProcess method?

tschechlovdev added the stupro label Aug 1, 2016

zesaro force-pushed the isbnfetcher branch 5 times, most recently from a7e7d7c to dcadfcf Compare August 1, 2016 13:33

tobiasdiez reviewed Aug 1, 2016
View reviewed changes

zesaro force-pushed the isbnfetcher branch 3 times, most recently from eded3fe to 58eea21 Compare August 8, 2016 11:22

tobiasdiez reviewed Aug 8, 2016
View reviewed changes

Siedlerchr reviewed Aug 10, 2016
View reviewed changes

zesaro force-pushed the isbnfetcher branch 2 times, most recently from 7a18d02 to 8764e52 Compare August 10, 2016 16:52

tschechlovdev added the stupro-ready-for-internal-review label Aug 12, 2016

zesaro added 3 commits August 13, 2016 14:01

implement isbnfetcher

badf5c3

create test

a823022

make fetcher usable and organise imports

1afaf62

koppor reviewed Aug 19, 2016
View reviewed changes

zesaro changed the title ~~IsbnFetcher~~ [WIP] IsbnFetcher Aug 22, 2016

stefan-kolb changed the title ~~[WIP] IsbnFetcher~~ IsbnFetcher Aug 22, 2016

koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 24, 2016

koppor changed the title ~~IsbnFetcher~~ ISBN Fetcher using the new fetcher infrastructure Aug 24, 2016

zesaro added 3 commits August 25, 2016 13:57

include feedback

f2c1571

Merge branch 'isbnfetcher' of github.com:zellerdev/jabref into isbnfe…

7a1583c

…tcher # Conflicts: # src/main/java/net/sf/jabref/gui/importer/fetcher/EntryFetchers.java # src/main/java/net/sf/jabref/logic/importer/fetcher/IsbnFetcher.java

refactor globals and fix logger

9615f10

zesaro force-pushed the isbnfetcher branch from 85b8500 to 9615f10 Compare August 25, 2016 16:13

oscargus reviewed Aug 25, 2016
View reviewed changes

zesaro force-pushed the isbnfetcher branch from bc6efe6 to 9615f10 Compare August 25, 2016 16:57

zesaro added 3 commits August 29, 2016 11:27

refactor globals in isbnfetcher

66b5dc8

Merge remote-tracking branch 'upstream/master' into isbnfetcher

c11e370

# Conflicts: # src/main/java/net/sf/jabref/gui/importer/fetcher/ISBNtoBibTeXFetcher.java

resolve merge issues

4887d8c

oscargus added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 29, 2016

stefan-kolb reviewed Aug 29, 2016
View reviewed changes

remove copyright

2ff9931

stefan-kolb merged commit d832acd into JabRef:master Aug 29, 2016

zesaro deleted the isbnfetcher branch August 29, 2016 10:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ISBN Fetcher using the new fetcher infrastructure #1654

ISBN Fetcher using the new fetcher infrastructure #1654

zesaro commented Aug 1, 2016 •

edited

Loading

tobiasdiez Aug 1, 2016

tobiasdiez Aug 8, 2016

tobiasdiez commented Aug 8, 2016

Siedlerchr Aug 10, 2016

koppor Aug 19, 2016

stefan-kolb commented Aug 23, 2016

oscargus commented Aug 23, 2016

stefan-kolb commented Aug 23, 2016

zesaro commented Aug 25, 2016

stefan-kolb commented Aug 25, 2016

oscargus Aug 25, 2016

oscargus Aug 27, 2016

oscargus commented Aug 25, 2016 •

edited

Loading

oscargus commented Aug 29, 2016

stefan-kolb Aug 29, 2016

koppor commented Sep 12, 2016

ISBN Fetcher using the new fetcher infrastructure #1654

ISBN Fetcher using the new fetcher infrastructure #1654

Conversation

zesaro commented Aug 1, 2016 • edited Loading

tobiasdiez Aug 1, 2016

Choose a reason for hiding this comment

tobiasdiez Aug 8, 2016

Choose a reason for hiding this comment

tobiasdiez commented Aug 8, 2016

Siedlerchr Aug 10, 2016

Choose a reason for hiding this comment

koppor Aug 19, 2016

Choose a reason for hiding this comment

stefan-kolb commented Aug 23, 2016

oscargus commented Aug 23, 2016

stefan-kolb commented Aug 23, 2016

zesaro commented Aug 25, 2016

stefan-kolb commented Aug 25, 2016

oscargus Aug 25, 2016

Choose a reason for hiding this comment

oscargus Aug 27, 2016

Choose a reason for hiding this comment

oscargus commented Aug 25, 2016 • edited Loading

oscargus commented Aug 29, 2016

stefan-kolb Aug 29, 2016

Choose a reason for hiding this comment

koppor commented Sep 12, 2016

zesaro commented Aug 1, 2016 •

edited

Loading

oscargus commented Aug 25, 2016 •

edited

Loading