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

Support \r when using CitationStyles #2165

Merged
merged 7 commits into from
Oct 16, 2016
Merged

Support \r when using CitationStyles #2165

merged 7 commits into from
Oct 16, 2016

Conversation

chriba
Copy link
Contributor

@chriba chriba commented Oct 15, 2016

Fixes: JabRef#174

The CitationStyle library cannot handle \r thus I parse to a space.
I also added another return (and localization) to always have a feedback for the user.

@boceckts boceckts added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed stupro-ready-for-internal-review labels Oct 15, 2016
@@ -56,17 +56,18 @@ protected static String generateCitation(BibEntry entry, String style, CitationS

} catch (IOException | ArrayIndexOutOfBoundsException e) {
LOGGER.error("Could not generate BibEntry Citation", e);
return Localization.lang("Cannot generate Citation");
Copy link
Member

Choose a reason for hiding this comment

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

I think, this generates the string displayed in the preview panel. Maybe, the message should be "Cannot generate preview based on selected citation style".

Further, "Citation" is not a fixed term, which should be uppercased. Put it lower case as most English words are lower case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@koppor
Copy link
Member

koppor commented Oct 15, 2016

Have you filed an upstream issue? You know, including minimal example. Similar to @obraliar did it for pgjdb-ng (https://github.com/obraliar/pgjdbc-ng-test#pgjdbc-ng-casing-test).

Maybe you also dived into the CSL library and could propose a fix for the lib (PR at their repo)

@koppor
Copy link
Member

koppor commented Oct 15, 2016

Is it possible to add a test case based on the example of JabRef#174. Further, a test case covering the exception branch should be made. Maybe using an invalid latex sequene in the entry helps to generate the exception?

Besides these minor comments: LGTM.

@chriba
Copy link
Contributor Author

chriba commented Oct 15, 2016

Sadly I cannot reproduce another case where the TokenMgrError is thrown.

I created an issue in the repository of the library: michel-kraemer/citeproc-java#30.

@koppor
Copy link
Member

koppor commented Oct 16, 2016

OK for me. - Either we keep this fix or include 1.0.0-SNAPSHOT as version of the library (see michel-kraemer/citeproc-java#30 (comment)).

@stefan-kolb
Copy link
Member

If I got this right, just use the SNAPSHOT version for now. Is a more long term solution using the fixed version instead of monkey-patching it?

@koppor koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Oct 16, 2016
@chriba
Copy link
Contributor Author

chriba commented Oct 16, 2016

Used the Snapshot which gets rid of the Rhino dependency as well.

@koppor koppor changed the title When generating the CitationStyle replace \r with space Support \r when using CitationStyles Oct 16, 2016
@koppor koppor merged commit 6903386 into JabRef:master Oct 16, 2016
@chriba chriba deleted the enhanceCitationStyleGenerator branch October 16, 2016 20:55
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.

4 participants