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

Upgrade EndNote XML exporter to StAX #11665

Closed
wants to merge 5 commits into from

Conversation

subhramit
Copy link
Collaborator

@subhramit subhramit commented Aug 22, 2024

Follow-up to #11157

Upgrade EndNote XML exporter

Refs. #11157 (comment)

  • Upgrades existing EndNote XML exporter to StAX for better memory efficiency.
  • Removes transformer to improve memory efficiency further (like Zotero).

Note: In the existing lambdas, now XMLStreamException is caught and thrown as RuntimeException as lambdas don't allow checked exceptions to be directly thrown from them.

Mandatory checks

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@subhramit subhramit changed the title Upgrade Endnote XML exporter to StAX Upgrade EndNote XML exporter to StAX Aug 22, 2024
@calixtus
Copy link
Member

About RuntimeException: ModsExporter also uses StAX and works with the so called SaveException. Does this maybe help?

@subhramit
Copy link
Collaborator Author

About RuntimeException: ModsExporter also uses StAX and works with the so called SaveException. Does this maybe help?

It did, took some inspiration. Used SaveException to handle the file export (the class mostly handles files unavailable for writing due to locking by another process).
Also added more precise exceptions to lambdas.

@subhramit
Copy link
Collaborator Author

subhramit commented Aug 22, 2024

Another prominent change - removed the transformer.
Tradeoff - can't pretty-print the XML anymore.
Benefit - even more memory efficient (for large databases) as we directly write to the output stream instead of building the unformatted xml in memory and then transforming it.
Zotero does it this way too.

@koppor
Copy link
Member

koppor commented Aug 24, 2024

Tradeoff - can't pretty-print the XML anymore.

Why? StackOverflow says different:

Benefit - even more memory efficient (for large databases) as we directly write to the output stream instead of building the unformatted xml in memory and then transforming it. Zotero does it this way too.

@koppor
Copy link
Member

koppor commented Aug 24, 2024

@Siedlerchr I find the new code less maintainable than the old one. In both codes, we do NOT need to generate Java-classes based on some XSD file. -- I propose to use this PR as learning for Subhramit for StAX, but close the PR due to maintenance concerns (see for example the necessary comment on closing XML elements).

@Siedlerchr
Copy link
Member

Yes that seems to be odd

@koppor
Copy link
Member

koppor commented Aug 26, 2024

DevCall: Longer discussion. We close. However, in case someone shows up with a large library and gets out of memory exceptions, we will reconsider.

@koppor koppor closed this Aug 26, 2024
@Siedlerchr Siedlerchr deleted the endnote-exporter-upgrade branch August 26, 2024 18:26
@Siedlerchr Siedlerchr restored the endnote-exporter-upgrade branch August 26, 2024 18:26
@subhramit subhramit deleted the endnote-exporter-upgrade branch September 7, 2024 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants