Fix #2455: Add crossref as fetcher for everything (ID-, entry-, search-based) #2645
Conversation
tobiasdiez
added
the
status: ready-for-review
| import org.jabref.model.FieldChange; | ||
| import org.jabref.model.cleanup.CleanupJob; | ||
| import org.jabref.model.entry.BibEntry; | ||
| import org.jabref.model.entry.FieldName; | ||
| import org.jabref.model.entry.identifier.ISSN; |
There was a problem hiding this comment.
For me an identifier is independent from the entry. org.jabref.model.identifier.ISSN
There was a problem hiding this comment.
I have put it there since other identifiers (ArXiv and MathSciNet) as well as other "field type classes" like month, authors and keywords are also placed there.
| @@ -24,96 +37,145 @@ | |||
| * | |||
| * See https://github.com/CrossRef/rest-api-doc | |||
| */ | |||
| public class CrossRef { | |||
| private static final Log LOGGER = LogFactory.getLog(CrossRef.class); | |||
| public class CrossRef implements IdParserFetcher<DOI>, EntryBasedParserFetcher, SearchBasedParserFetcher, IdBasedParserFetcher { | |||
There was a problem hiding this comment.
To be honest, the hierarchy of interfaces etc. has gotten way to complicated for the fetchers. I think its really hard to get an idea whats happening right now. With this PR it even got harder to keep track.
There was a problem hiding this comment.
And I'm saying this having written a lot of fetcher code back then
There was a problem hiding this comment.
Mhhh....
- IdFetcher: gets an ID for a BibEntry
- EntryBased: gets more information for a BibEntry
- SearchBased: gets BibEntries based on a free-text search query
- IdBased: gets a BibEntry from an identifier
I'm not sure how to simplify this. These interfaces each cover different aspects and not all fetcher have all the capabilities. Do you have any suggestions?
There was a problem hiding this comment.
Well, these are not all interfaces as there are the ...Parser... interfaces, the WebFetcher, the FullTextFetcher etc. so in total this is really complex at the moment...
Maybe a better solution would be to reuse the parsing logic by composition instead of introducing further interfaces?
There was a problem hiding this comment.
Instead of the ...Parser...interfaces, I could imagine a syntax similar to
Optional<BibEntry> performSearchById(String identifier) throws FetcherException {
return FetcherBuilder.invoke(getURLForID(identifier))
.parse(this::getParser)
.cleanup(this::doPostCleanup)
.fetchSingle();What do you think?
(Anyway, the aim of this PR was to add a single fetcher not to refactor everything)
There was a problem hiding this comment.
My opinion on this: Don't combine all interfaces to a single one. The purpose of an Interface is to provide specific methods/behaviour.
If one concrete fetcher has multiple behavior, e.g. can fetch IDs as well as FullText, it should implement both interfaces.
Example:
MycombinedFetcher implements IDBasedFetcher, FullTextFetcher
public String getFullText()
public string getEntryFromID()
There was a problem hiding this comment.
I mean you obviously worked a lot on this, so it is not easy to give a good suggestion. I'll try to take a look on this in the next days. If I am not able to come up with a good solution we will keep it the way it is.
There was a problem hiding this comment.
I would be very happy if you find a solution where the ...Parser ... interfaces are not needed. I never really liked them. I'll merge this PR now but will have course help to rewrite the fetchers once you found a nice solution.
| @@ -41,4 +43,16 @@ | |||
| public static <T, R> Stream<R> flatMap(Optional<T> value, Function<? super T, ? extends Collection<? extends R>> mapper) { | |||
| return toStream(value).flatMap(element -> mapper.apply(element).stream()); | |||
| } | |||
|
|
|||
| public static <T> Boolean isPresentAnd(Optional<T> value, Predicate<T> check) { | |||
There was a problem hiding this comment.
Sometimes the use of optional is a bit inconvenient and redundant but for me these helper methods add additional complexity and twist my head while reading the code.
There was a problem hiding this comment.
I also don't really like them but still found them more readable than the inlined code. I will keep them for the moment but feel free to remove them if you really dislike.
|
Also the diff is hardly reviewable, tho this is not your fault. |
Amounted to an almost-rewrite of the existing crossref fetcher, then adding the new functionality was done in 5 lines 😄 . Besides this change I have done a few refactorings related to fetchers and the DOI:
more fetcher things in logic (EntryFetchers -> WebFetchers) and moved the identifier classes from logic to model.
(This PR also contains commits from my previous PR...since, well, I will never learn it).
gradle localizationUpdate?