-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
ADS Fetcher using the new fetcher infrastructure #1923
Conversation
6089cb7
to
eff8deb
Compare
|
||
try { | ||
URIBuilder uriBuilder = new URIBuilder(URL_PATTERN + key); | ||
uriBuilder.addParameter("data_type", "BIBTEX"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you pass "BIBTEXPLUS" as the data_type, then the abstract is also included and the second request below is unnecessary.
Good timing! In #1929, I introduce the search-query-based ADS fetcher. Since some of the (apparently) open problems are already solved in #1929 (like the post-processing), I think it is best to wait with this PR until #1929 is merged. Or you create a PR onto tobiasdiez:newFetcher, whatever you like the best. |
Could you please integrate your AdsFetcher class with the new |
# Conflicts: # src/main/java/net/sf/jabref/gui/importer/fetcher/EntryFetchers.java # src/test/java/net/sf/jabref/logic/importer/fetcher/IsbnFetcherTest.java
Travis fails the searchByQueryFindsEntry Test due to an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beside some minor comments, LGTM.
@@ -90,6 +93,19 @@ public URL getURLForEntry(BibEntry entry) throws URISyntaxException, MalformedUR | |||
} | |||
|
|||
@Override | |||
public URL getURLForID(String identifier) throws URISyntaxException, MalformedURLException, FetcherException { | |||
String key = identifier.replaceAll("^(doi:|DOI:)", ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please extract pattern
+ "problems of this alternative paradigm at all astrophysical scales, and" + NEWLINE | ||
+ "summarize the various theoretical attempts (TeVeS, GEA, BIMOND, and" + NEWLINE | ||
+ "others) made to effectively embed this modification of Newtonian" + NEWLINE | ||
+ "dynamics within a relativistic theory of gravity."+ NEWLINE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Abstracts are copyright protected... Whether you can remove it or you have to take an other entry.
@Test(expected = FetcherException.class) | ||
public void testPerformSearchByIdInvalidDoi() throws Exception { | ||
Optional<BibEntry> fetchedEntry = fetcher.performSearchById("this.doi.will.fail"); | ||
assertEquals(Optional.empty(), fetchedEntry); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the assertEquals. You expect a exception and not an empty optional right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you then have to use the fail(), otherwise it will produce a codecov or warning in Eclipse for about the test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I replaced the assertEquals() with fail() and removed fetchedEntry. so just performSearch() is called :)
* create new fetcher and test * remove old fetcher * remove obsolete keys * add new fetcher in EntryFetchers * include feedback use BIBTEXPLUS as type * remove useless curly brackets * rename test cases * use IdBasedParserFetcher, update adsfetcher and test, add to entryfetchers * integrate AdsFetcher into AstrophysicsDataSystem * fix localization * modify remove doi pattern and testPerformSearchByIdEmptyDOI * remove abstract in existing entries and add new without any abstract by default
Recreate the ADS Fetcher from ADS using the new fetcher infrastructure #1594