Skip to content

Conversation

@zacmks
Copy link
Contributor

@zacmks zacmks commented Jul 18, 2017

Fixed the following issue:

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

Also, update the maven repo, with secure http

@koppor
Copy link
Member

koppor commented Jul 18, 2017

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.

Copy link
Member

@LinusDietz LinusDietz left a comment

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

build.gradle Outdated
repositories {
maven {
url 'http://maven.ej-technologies.com/repository'
url 'https://maven.ej-technologies.com/repository'
Copy link
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
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
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
Copy link
Contributor Author

zacmks commented Jul 18, 2017

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!

Copy link
Member

@Siedlerchr Siedlerchr left a comment

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