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

Change latex command handling only for XML Chars #4379

Merged
merged 8 commits into from
Mar 27, 2019

Conversation

johannes-manner
Copy link
Collaborator

@johannes-manner johannes-manner commented Oct 16, 2018

Refs #3838

  • Change in CHANGELOG.md described
  • Tests created for changes
  • Manually tested changed features in running JabRef

Tried to solve this issue in PR #4108.
This approach is now more dedicated to the XML docbook export and not applied to all fields as the first PR was.

I am also not sure, if this issue is related to #3781, #3644?

Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

Thanks for the follow-up. The code change looks good to me. Can you please add a changelog entry and a test verifying that the issue is fixed indeed. Thanks!

@johannes-manner
Copy link
Collaborator Author

@bernhard-kleine: I exported your provided bib entry via docbooks exporter into XML. Please check the result and let us know, if there is something missing.

testBracesResult.xml.txt
testbraces.bib.txt

I will write tests based on your files, if the export is now correct.

@bernhard-kleine
Copy link

Thank you @johannes-manner, this looks good to me. For the testcase, maybe we could skip those entries where there are no latex braces in the title.

@bernhard-kleine
Copy link

bernhard-kleine commented Oct 17, 2018

This is a separate item from the docbook5 export #4319 where I am waiting for feedback.

@johannes-manner
Copy link
Collaborator Author

Added two test cases for (1) removed words based on an error during the latex command processing and for (2) unicode problems based on the same error.

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.

Overall looks good! Thank you for going the extra mile and adding the tests.

I have some really minor comments, which are by no means a blocker for merging this PR.

}

@AfterEach
public void tearDown() {
Copy link
Member

Choose a reason for hiding this comment

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

Is the tearDown method really necessary?


@BeforeEach
public void setUp() {
Map<String, TemplateExporter> customFormats = new HashMap<>();
Copy link
Member

Choose a reason for hiding this comment

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

how about inlining customFormats?

exportFormat = exporterFactory.getExporterByName("docbook").get();

databaseContext = new BibDatabaseContext();
charset = StandardCharsets.UTF_8;
Copy link
Member

Choose a reason for hiding this comment

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

You can move databaseContext and charset initialization into the field, so the setUp method is less complex.

@LinusDietz
Copy link
Member

Ping. What is the status @johannes-manner?

@johannes-manner
Copy link
Collaborator Author

Oversee the comments and the ping. Sorry for that :)

Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

Thanks!

@tobiasdiez tobiasdiez merged commit 44163bf into JabRef:master Mar 27, 2019
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.

None yet

5 participants