-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Can't Save a Database to a new File #3022
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
Conversation
|
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. |
There was a problem hiding this 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' |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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/
|
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! |
There was a problem hiding this 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
|
@zacmks Good work! I'll merge this now. Will you open the other pull request for the https maven repository? |
Fixed the following issue:
https://github.com/zacmks/jabref/issues/1
Also, update the maven repo, with secure http