Skip to content

Can't Save a Database to a new File#3022

Merged
LinusDietz merged 5 commits into
JabRef:masterfrom
zacmks:es2
Jul 18, 2017
Merged

Can't Save a Database to a new File#3022
LinusDietz merged 5 commits into
JabRef:masterfrom
zacmks:es2

Conversation

@zacmks

@zacmks zacmks commented Jul 18, 2017

Copy link
Copy Markdown
Contributor

Fixed the following issue:

https://github.com/zacmks/jabref/issues/1

Also, update the maven repo, with secure http

@koppor

koppor commented Jul 18, 2017

Copy link
Copy Markdown
Member

Thank you for the PR. I think, our checkstyle check will result in errors. We are aware that checkstyle may be seen as annoying (#2996). Our aim is to have the same code style over the whole project and thus, we decided to keep checkstyle in place. We currently have no howto hosted at the JabRef dev doc, but the howto at another project shows how to include checkstyle in IntelliJ: http://eclipse.github.io/winery/dev/config/IntelliJ%20IDEA/README.html.

@LinusDietz LinusDietz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hey @zacmks, thank you for your Pull Request! You have identified a really embarrassing problem and we are very happy, that you already submitted a solution for this. (:

I have some remarks that I think can be fixed quite easily before this PR can be merged into master.
Also please make sure that the checkstyle warnings are fixed. you can run them using gradle check

Comment thread build.gradle Outdated
repositories {
maven {
url 'http://maven.ej-technologies.com/repository'
url 'https://maven.ej-technologies.com/repository'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a very good point, and @koppor and me don't see any downside for using https. Despite that, this has nothing to do with the rest this PR, so I would like you to open another PR for this.

zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-4.0.1-bin.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-4.0.1-all.zip

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please keep this file as before and adjust your IDE settings accordingly.

AutosaveManager.shutdown(context);
BackupManager.shutdown(context);
} catch (NoSuchElementException e) {
LOGGER.info("Old file not found, just creating a new file", e);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

context.getDatabasePath().get() returns an Optional which you can check for being present/absent using isPresent(). Then you won't need a try-catch block.

Check out this page to learn the most important features of Optionals https://examples.javacodegeeks.com/core-java/util/optional/java-8-optional-example/

@LinusDietz LinusDietz changed the title Es2 Can't Save a Database to a new File Jul 18, 2017
@zacmks

zacmks commented Jul 18, 2017

Copy link
Copy Markdown
Contributor Author

Sorry guys, @koppor, @lynyus. Should be fixed on the latest commit. Not very used to all these tools, I'm used to use JIRA, GitLab, and formatting with IDE messed up with style, sorry. I was just doing some class stuff and found the issue (next time, I'll check more carefully the projects' requirements). And thank you very much for the tips!

@Siedlerchr Siedlerchr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you very much for your contribution!This looks good now

@LinusDietz

Copy link
Copy Markdown
Member

@zacmks Good work! I'll merge this now. Will you open the other pull request for the https maven repository?

@LinusDietz LinusDietz merged commit c08c5d4 into JabRef:master Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants