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.

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

Made BibDatabaseContext.getDatabaseFile() return Optional #1830

Merged
merged 1 commit into from
Aug 26, 2016

Conversation

oscargus
Copy link
Contributor

Plus one (or two, I couldn't confirm the second but I think there will be an NPE if starting JabRef with an already existing file as an argument and an unsaved (untitled) database open) bug fixes.

This also includes #1782 as I messed up a bit...

  • Change in CHANGELOG.md described
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)

@oscargus oscargus added bug Confirmed bugs or reports that are very likely to be bugs status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers type: code-quality Issues related to code or architecture decisions labels Aug 23, 2016
@@ -175,13 +175,14 @@ private String getFileDirectoryPath(String directoryName) {
String dir = directoryName;
// If this directory is relative, we try to interpret it as relative to
// the file path of this BIB file:
if (!new File(dir).isAbsolute() && (getDatabaseFile() != null)) {
Optional<File> databaseFile = getDatabaseFile();
if (!new File(dir).isAbsolute() && databaseFile.isPresent()) {
Copy link
Member

@Siedlerchr Siedlerchr Aug 23, 2016

Choose a reason for hiding this comment

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

I am not quite sure about this one, but if you use the nio Paths methods, you maybe could omit the check for absolute/relative directory.

Edit// Yeah I looked at the whole method here and I think it could be done with Paths.get and getParent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. I have very little (basically no) idea about nio.Paths though, so I prefer to avoid it at this time... If you could spare some time and provide code I can just paste it would be appreciated though.

} else {
if (!uniqPath.equals(file.getName())) {
if (file.isPresent()) {
if (!uniqPath.equals(file.get().getName())) {
// remove filename
uniqPath = uniqPath.substring(0, uniqPath.lastIndexOf(File.separator));
Copy link
Member

Choose a reason for hiding this comment

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

Just Paths.get().getParent()

@Siedlerchr
Copy link
Member

Just some minor nio related things you could change, too. Apart from that, the optional stuff LGTM 👍

@oscargus
Copy link
Contributor Author

I'm not confident enough regarding nio.Paths to edit blindly (many other APIs I do that though), nor do I really know how to trigger the code correctly, so I would very much prefer to leave it as is. Even though I conceptually understand at least the last comment and see that makes sense to use it there.

@oscargus
Copy link
Contributor Author

I merge this in and hope @Siedlerchr are helpful at some stage regarding the nio-stuff...

@oscargus oscargus merged commit c609376 into JabRef:master Aug 26, 2016
ayanai1 pushed a commit to ayanai1/jabref that referenced this pull request Sep 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bugs or reports that are very likely to be bugs status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers type: code-quality Issues related to code or architecture decisions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants