Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
mEDRA DOI fetcher implementation. #6641
mEDRA DOI fetcher implementation. #6641
Changes from 3 commits
2d75da2
df31be7
b521e7c
3539079
1b78fd5
81d5cc3
51423cb
a42904c
c0a329f
85fc98f
2ede4ac
a108974
4f96aab
4cfc672
4ae1788
8c7f1b5
5504e52
6fd81c3
9b9b14f
3596442
d17af0f
dd0a665
4fa073e
faa304a
4523f1d
1793870
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
We try to avoid null. Instead you can use Optionals.
https://blog.indrek.io/articles/optionals-in-java-8/
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.
Could be something like this?
and the condition in performSearchById would then be
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.
You need to check if the value is present first:
getAgency(...).ifPresent() && getAgency(...).get().equals(medra)
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.
Yes, sure, and I also needs to change
agency = Optional.of(response.optString("RA"));
toagency = Optional.ofNullable(response.optString("RA"));
since optString could return a null.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.
Do you really need the SearchBasedParserFetcher interface?
I think this is superflous. The IDBasedFetcher should be sufficient
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.
Ah yes, I forgot to remove it.
In this case I don't see why i should add Medra Fetcher to org.jabref.logic.importer.WebFetchersTest array test, since it seems that this already loads Medra class to expected array of
getIdBasedFetchersReturnsAllFetcherDerivingFromIdBasedFetcher()
and the test is passed.Am I missing anything?
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.
That was probaby the reason for the failing test at first. If it passes now it's fine. Nothing to do then.
Please take a look at the checkstyle issues, than it's ready to go!
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.
Maybe I fixed the style issues and wanted to run
./gradlew checkstyleMain checkstyleTest checkstyleJmh
from terminal to test them before committing but i get:running with --info gives some more details:
Should I commit anyway and see what the tests say?
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.
Yes, just commit.
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.
These 4 lines can be done shorter as it is a single entry:
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.
Ah, you mean HTTP 404 returns an empty response.
In line 259 of URLDownload is is implemented exactly as that.
Why do the other fetchers cope well with that and here you have to do some special tweaks?
Maybe, the response from the mEDRA thing is different?
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.
You're right, URLDownload returns empty stream but I thought somewhere I had to check whether it's empty or not before trying to parse it to JSON.
Crossref serach by ID doesn't cope well with that, for example, because there's not empty stream check. I added a test for valid DOI returning nothing to CrossRefTest and the test failed.
If you think this could be the case I could implement just that check on performSearchById of Crossref as well, otherwise i just commit my changes.
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 it makes sense to add that to CrossRef as well
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, this is fixed in https://github.com/mind000/jabref/pull/1/, too.