Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix #2427: Arxiv fetcher works with prefix #2428

Merged
merged 1 commit into from
Dec 24, 2016
Merged

Fix #2427: Arxiv fetcher works with prefix #2428

merged 1 commit into from
Dec 24, 2016

Conversation

tobiasdiez
Copy link
Member

  • Change in CHANGELOG.md described
  • Tests created for changes
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)
  • If you changed the localization: Did you run gradle localizationUpdate?

@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 22, 2016
stefan-kolb
stefan-kolb requested changes Dec 22, 2016
View reviewed changes
src/main/java/net/sf/jabref/logic/importer/fetcher/ArXiv.java
@@ -107,6 +107,10 @@ public ArXiv(ImportFormatPreferences importFormatPreferences) {
}

private Optional<ArXivEntry> searchForEntryById(String identifier) throws FetcherException {
identifier = identifier.replace("arxiv:", "");
identifier = identifier.replace("arXiv:", "");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you just replace this via a regex? /arxiv:?/i

Copy link
Contributor

@simonharrer simonharrer Dec 22, 2016 •
edited by stefan-kolb
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would vote for an ArXivID class, similar to the DOI one which can handle such a case.

src/test/java/net/sf/jabref/logic/importer/fetcher/ArXivTest.java
@@ -76,6 +76,14 @@ public void findByEprint() throws IOException {
}

@Test
// Test for https://github.com/JabRef/jabref/issues/2427
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would omit the comment here. It is a test for expected behavior. Doesnt matter which bug/problem it fixed.

src/test/java/net/sf/jabref/logic/importer/fetcher/ArXivTest.java
@@ -139,6 +147,12 @@ public void searchEntryByIdWith4Digits() throws Exception {
}

@Test
// Test for https://github.com/JabRef/jabref/issues/2427
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

Siedlerchr
Siedlerchr reviewed Dec 23, 2016
View reviewed changes
src/test/java/net/sf/jabref/logic/importer/fetcher/ArXivTest.java
// Test for https://github.com/JabRef/jabref/issues/2427
public void findByEprintWithPrefix() throws IOException {
entry.setField("eprint", "arXiv:1603.06570");

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove empty line

Siedlerchr
Siedlerchr reviewed Dec 23, 2016
View reviewed changes
src/test/java/net/sf/jabref/logic/importer/fetcher/ArXivTest.java
@@ -139,6 +147,12 @@ public void searchEntryByIdWith4Digits() throws Exception {
}

@Test
// Test for https://github.com/JabRef/jabref/issues/2427
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove url

@koppor koppor merged commit 8be62e0 into master Dec 24, 2016
@koppor
Copy link
Member

koppor commented Dec 24, 2016

I merged it and will do the requested changes now to get this into 3.8.2.

@koppor koppor deleted the arxivPrefix branch December 24, 2016 13:31
koppor added a commit that referenced this pull request Dec 24, 2016
@koppor
Include feedback
4d56f10 
Source: #2428
@tobiasdiez tobiasdiez mentioned this pull request Dec 28, 2016
Merged
6 tasks
tobiasdiez added a commit that referenced this pull request Jan 15, 2017
@tobiasdiez
Follow up #2428 (#2438)
f57a2e0 
* Create ArXivIdentifier class

* Add arXiv ID-based fetcher (and tests)

* Fix style

* Add end of file line breaks
Siedlerchr added a commit that referenced this pull request Jan 18, 2017
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into moveFileDir
0ca9e1d 
* upstream/master:
  Save deletion of current searchquery (#2469)
  Update dev dependencies (mockito, wiremock, assertj)
  Update BouncyCastle (1.55->1.56), ANTRL4 (4.5.3->4.6), jsoup (1.10.1->1.10.2)
  Group all checker which only check the value of one field (#2437)
  Follow up #2428 (#2438)
  Fix conversion of "'n" and "\'{n}" from LaTeX to Unicode (#2464)
  Fix failing tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonharrer simonharrer simonharrer left review comments

@Siedlerchr Siedlerchr Siedlerchr left review comments

@stefan-kolb stefan-kolb stefan-kolb requested changes

Assignees
No one assigned
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tobiasdiez @koppor @Siedlerchr @simonharrer @stefan-kolb