Refactor ModsExporter to use StaX from JAXB #9740

yenniejunvu · 2023-04-05T15:45:53Z

Some starting changes to ModsExporter, refactoring from using JAXB to StAX. Not done yet, just wanted to commit some initial changes.

Fix for #9682

Compulsory checks

Give feedback

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.
Options

yenniejunvu · 2023-04-05T16:49:37Z

Alternatively, I could get rid of modsCollection altogether and write directly to the export file in each respective 'add' method. I would call createStaxWriter first, before the loop.

yenniejunvu · 2023-04-09T21:38:51Z

Some solid progress for refactoring, but need to figure out a different way to handle OriginInformation and RelatedInformation

yenniejunvu · 2023-04-12T16:40:22Z

Working on fixing checkstyle and cleaning up the code

src/main/java/org/jabref/logic/exporter/ModsExporter.java

Siedlerchr · 2023-04-12T20:55:00Z

src/main/java/org/jabref/logic/exporter/ModsExporter.java

+                        addIdentifier(writer, new UnknownField("citekey"), citeKey);
+                    } catch (
+                            XMLStreamException e) {
+                        throw new RuntimeException(e); // cant throw SaveException?


This looks a bit odd to me. What is the purpose?
Why not use the isPresent? Or set it to an empty string? e.g. ifPresent.OrElse(...)

Are you asking about the purpose of using try-catch here? Or the use of ifPresent(citekey ... ?

I used ifPresent because that is what the code had originally.

Both. First question: Why do you need the cry catch? I would assume if there is no citeKey we set it to empty string or something like this and the other line does a similar check down below. So it's kind of duplicated code

Siedlerchr · 2023-04-21T22:26:26Z

Changelog entries should be understandable for the normal average user, they can't do anything with the technical detils

koppor · 2023-04-22T03:41:47Z

Changelog entries should be understandable for the normal average user, they can't do anything with the technical detils

We don't need a changelog entry here as there is no functionality changed, is it?

koppor · 2023-04-22T03:43:50Z

Working on fixing checkstyle and cleaning up the code

Regarding checkstyle: You can click on https://github.com/JabRef/jabref/pull/9740/files to directly see the checkstyle comments. Moreover, IntelliJ has a build-in checkstyle plugin. The setup is explained at https://devdocs.jabref.org/getting-into-the-code/guidelines-for-setting-up-a-local-workspace. Search for "checkstyle".

If everything works, checkstyle checking can be started with one click:

koppor

Some small comments.

Are there tests for the modsexporter? I saw some TODOs in the code - is the expected output covered by test cases?

koppor · 2023-04-29T00:16:31Z

src/main/java/org/jabref/logic/exporter/ModsExporter.java

-import org.jabref.logic.importer.fileformat.mods.TitleInfoDefinition;
-import org.jabref.logic.importer.fileformat.mods.TypeOfResourceDefinition;
-import org.jabref.logic.importer.fileformat.mods.UrlDefinition;
+// relevant StAX imports


No need to comment on this, just remove that.

CHANGELOG.md

koppor · 2023-04-29T00:18:24Z

src/main/java/org/jabref/logic/exporter/ModsExporter.java

@@ -76,267 +49,343 @@ public void export(final BibDatabaseContext databaseContext, final Path file, Li
        }

        try {
-            ModsCollectionDefinition modsCollection = new ModsCollectionDefinition();
+            // create writer
+            IndentingXMLStreamWriter writer = createWriter(file);


I think, the writer should be in the "try" statement - to have it auto closable:

try (IndentingXMLStreamWriter writer = createWriter(file))

Does not work! StreamWriter does not support auto closable.

koppor · 2023-04-29T00:18:59Z

src/main/java/org/jabref/logic/exporter/ModsExporter.java

+        writer.flush();
+        writer.close();


Obsolete at try-with-resources.

As mentionend earlier, java's XMLWriter do not implement auto closable

Now there is a JavaComment on that. Thank you!

src/main/java/org/jabref/logic/exporter/ModsExporter.java

Siedlerchr · 2023-05-06T17:16:16Z

What is the status here? It would be great if you continue working on this.

* upstream/main: (110 commits) remove extra hack rather add a todo Enable CompareEnumsWithEqualityOperator cleanup checkstyle checkstyle Prefer u over y, remove some patterns that have not a test yet make parameterized test Enable ChainStringBuilderAppendCalls Add four rules not having any effect Apply BooleanChecksNotInverted Remove unused RadioButtonCell move exception handling upwards Minimal config for openRewrite Fix missing # Update CHANGELOG.md exchange locations between library mode and library encoding added to Changelog exchange locations between library mode and library encoding in general tab Fix modernizer (JabRef#9824) CHANGELOG.md Removed unused code Refined ui ... # Conflicts: # src/main/java/org/jabref/logic/exporter/ModsExporter.java

…m.sun.xml set indendation and standalone

created XMLStreamWriter

8021162

yenniejunvu changed the title ~~created XMLStreamWriter~~ Refactor ModsExporter to use StaX from JAXB Apr 5, 2023

progress, but changes needed for origin info and related info

b727b2a

some format fixes

cdafb21

fixed checkstyle

305a75b

yenniejunvu marked this pull request as ready for review April 12, 2023 20:09

Siedlerchr reviewed Apr 12, 2023

View reviewed changes

src/main/java/org/jabref/logic/exporter/ModsExporter.java Outdated Show resolved Hide resolved

Siedlerchr reviewed Apr 12, 2023

View reviewed changes

yennie02 and others added 2 commits April 21, 2023 16:19

added changelog, removed try catch

bff33d8

Merge branch 'main' into fix-for-9682

7c27c46

yennie02 added 2 commits April 22, 2023 11:34

removed changelog entry, fixed checkstyle and unit tests

9f2061e

changelog merge

a3c9e62

koppor added the status: changes required Pull requests that are not yet complete label Apr 24, 2023

Merge branch 'main' into fix-for-9682

8164ea1

koppor requested changes Apr 29, 2023

View reviewed changes

Siedlerchr added 4 commits May 7, 2023 19:31

refactor to use Transformer with string writer in between to avoid co…

4ba4ac4

…m.sun.xml set indendation and standalone

remove changelog

e0869f1

fix checkstyle

6163774

Siedlerchr added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed status: changes required Pull requests that are not yet complete labels May 8, 2023

Siedlerchr added 2 commits May 8, 2023 22:07

remove comments

47eae38

remove comments

82dcfbf

koppor approved these changes May 8, 2023

View reviewed changes

calixtus approved these changes May 8, 2023

View reviewed changes

koppor merged commit 68b72e0 into JabRef:main May 8, 2023

koppor mentioned this pull request May 8, 2023

Remove generateModsSource (and mods-3-7.xsd) #9860

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor ModsExporter to use StaX from JAXB #9740

Refactor ModsExporter to use StaX from JAXB #9740

yenniejunvu commented Apr 5, 2023

Compulsory checks

yenniejunvu commented Apr 5, 2023

yenniejunvu commented Apr 9, 2023

yenniejunvu commented Apr 12, 2023

Siedlerchr Apr 12, 2023

yenniejunvu Apr 12, 2023

Siedlerchr Apr 15, 2023

Siedlerchr commented Apr 21, 2023

koppor commented Apr 22, 2023

koppor commented Apr 22, 2023

koppor left a comment

koppor Apr 29, 2023

koppor Apr 29, 2023

Siedlerchr May 7, 2023

koppor Apr 29, 2023

Siedlerchr May 7, 2023

koppor May 8, 2023

Siedlerchr commented May 6, 2023

Refactor ModsExporter to use StaX from JAXB #9740

Refactor ModsExporter to use StaX from JAXB #9740

Conversation

yenniejunvu commented Apr 5, 2023

Compulsory checks

yenniejunvu commented Apr 5, 2023

yenniejunvu commented Apr 9, 2023

yenniejunvu commented Apr 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr commented Apr 21, 2023

koppor commented Apr 22, 2023

koppor commented Apr 22, 2023

koppor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr commented May 6, 2023