Skip to content

Fix exporting via commandline in no gui mode#2316

Merged
matthiasgeiger merged 3 commits into
masterfrom
cmdexport
Nov 29, 2016
Merged

Fix exporting via commandline in no gui mode#2316
matthiasgeiger merged 3 commits into
masterfrom
cmdexport

Conversation

@Siedlerchr

@Siedlerchr Siedlerchr commented Nov 28, 2016

Copy link
Copy Markdown
Member

Fix for #2273
Adjusted javadoc

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

@Siedlerchr Siedlerchr added component: export-or-save status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Nov 28, 2016

@tobiasdiez tobiasdiez 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.

In general LGTM, just a few questions/remarks from my side.

Comment thread CHANGELOG.md Outdated
- We fixed an issue when JabRef restores its session and a shared database was used: The error message "No suitable driver found" will not appear.
- Update check now correctly notifies about new release if development version is used. [#2298](https://github.com/JabRef/jabref/issues/2298)
- Fixed [#2311](https://github.com/JabRef/jabref/issues/2311): The DBLP fetcher has been rewritten and is working again.
- Fixed [#2273](https://github.com/JabRef/jabref/issues/2273): Export via commandline in no-gui modus is now working again.

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.

modus -> mode

} catch (Exception ex) {
System.err.println(Localization.lang("Could not export file") + " '" + data[1] + "': "
+ ex.getMessage());
+ ExceptionUtils.getStackTrace(ex));

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.

Did you changed this on purpose? All the other error reporting in this class seems to display the message instead of the stack trace (not sure what is better, but it should be consistent).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, because if you look into the issue, ex.getMessage simply returned "null" and there was no stacktrace visible, which made it hard to locate the bug.
Could not export file 'references.html': null

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I looked at all other cases and there is either a logger attached or a specific Exception, e.g. a SaveException/JabRef Exception catched and the message printed, which is fine enough.
Only this two line were problematic, as the getMessage returned null

final Charset encoding, List<BibEntry> entries) throws Exception {
Objects.requireNonNull(databaseContext);
Objects.requireNonNull(entries);
Objects.requireNonNull(databaseContext, "Database Context must not be null!");

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.

To be honest, in my opinion these string messages don't add more relevant information.

* upstream/master:
  Cleanup EMACS code (#2317)
  Update mockito-core from 2.2.15 to 2.2.21
  Fix typo in comment
Remove string messages from requireNonNull
@Siedlerchr

Copy link
Copy Markdown
Member Author

Included feedback

@matthiasgeiger matthiasgeiger merged commit 5d797b2 into master Nov 29, 2016
@matthiasgeiger matthiasgeiger deleted the cmdexport branch November 29, 2016 20:04
Siedlerchr added a commit that referenced this pull request Dec 2, 2016
* upstream/master:
  Ignore failing test
  Replace usage of Threads and priorities with thread pool (#2304)
  Class variable declarations and method declarations are now separated by one line
  Disable joining of wrapped lines
  Installer Code Signing #1879 (#2320)
  Add bibtex key deviation check (#2328)
  Update mockito-core (2.2.21 -> 2.2.26) and wiremock (2.3.1 -> 2.4.1)
  Fix opening of preference dialog with Java 9 (#2329)
  Add longer explanation for ID-based entry generation. (#2330)
  Add DOI integrity check (#2327)
  New strings translated (#2325)
  Fix exporting via commandline in no gui mode (#2316)
  Cleanup EMACS code (#2317)
  Update mockito-core from 2.2.15 to 2.2.21
  Fix typo in comment
  Updated JabRef_tr.properties (#2315)

# Conflicts:
#	CHANGELOG.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: export-or-save status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants