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

Make path relative when adding automatically found file #3551

Merged
merged 2 commits into from
Dec 21, 2017
Merged

Make path relative when adding automatically found file #3551

merged 2 commits into from
Dec 21, 2017

Conversation

Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Dec 19, 2017 •
edited
Loading

Fixes #3476

  • 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?

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 19, 2017
@Siedlerchr Siedlerchr changed the title Make path relatvie when adding automatically found file Make path relative when adding automatically found file Dec 19, 2017
lenhard
lenhard approved these changes Dec 19, 2017
View reviewed changes
Copy link
Member

@lenhard lenhard left a comment

Choose a reason for hiding this comment

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

Haven't tested it locally, but the code looks fine and you also extended the tests. Thanks!

tobiasdiez
tobiasdiez requested changes Dec 19, 2017
View reviewed changes
Copy link
Member

@tobiasdiez tobiasdiez left a comment •
edited
Loading

Choose a reason for hiding this comment

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

As @lenhard, I was too lazy to test the code. Aside from a small remark concerning the test, this is good to go. You can merge directly.

src/test/java/org/jabref/gui/externalfiles/AutoSetFileLinksUtilTest.java Outdated
when(externalFileTypes.getExternalFileTypeSelection()).thenReturn(new TreeSet<>(externalFileTypes.getDefaultExternalFileTypes()));

}

@Test
public void test() {
//Due to mocking the externalFileType class, the file extension will not be found
List<LinkedFile> expected = Collections.singletonList(new LinkedFile("", file.toString(), ""));

String relativeFilePath = FileUtil.shortenFileName(file, dirs).toString();
Copy link
Member

Choose a reason for hiding this comment

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

Well, now the test uses the same logic as the method under test. Could you please hard code the expected value. It should be just CiteKey.pdf, right?

@LinusDietz LinusDietz merged commit 55abd5e into master Dec 21, 2017
@LinusDietz LinusDietz deleted the fixautolinkrelativefile branch December 21, 2017 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tobiasdiez tobiasdiez tobiasdiez requested changes

@lenhard lenhard lenhard approved these changes

@LinusDietz LinusDietz LinusDietz approved these 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.
4 participants
@Siedlerchr @LinusDietz @lenhard @tobiasdiez