Refactor export code to fix #3576 by tobiasdiez · Pull Request #3578 · JabRef/jabref

tobiasdiez · 2017-12-25T14:06:11Z

Fixes #3576.
I refactored the export package so that it now has a similar structure of the importer and a similar code can now be used for imports and exports. Since the issue #3576 didn't occurred for imports, this refactoring automatically fixes the problem for the export.

Notable changes:

ExportFormat is renamed to Exporter
ExportFormats is renamed to ExportFactory and no longer static.
FileExtensions is renamed to FileType

Change in CHANGELOG.md described
Tests created for changes
Screenshots added (for bigger UI changes)
Manually tested changed features in running JabRef
Check documentation status (Issue created for outdated help page at help.jabref.org?)
If you changed the localization: Did you run gradle localizationUpdate?

Siedlerchr · 2017-12-25T16:38:42Z

+ * TemplateExporter for exporting in MODS XML format.
 */
-class ModsExportFormat extends ExportFormat {
+class ModsExporter extends TemplateExporter {


I don't know by what you define a TemplateExporter: If it's the one where you can define a html/layout file, then this is not one.

I now made Exporter an abstract class with the basic fields. Not sure why these special exporter derived from ExportFormat.

Siedlerchr · 2017-12-25T16:39:02Z

+ * TemplateExporter for exporting in MSBIB XML format.
 */
-class MSBibExportFormat extends ExportFormat {
+class MSBibExporter extends TemplateExporter {


Not a Template exporter. You can't create a layout file for it

Siedlerchr · 2017-12-25T16:54:02Z

+    private static void storeOpenDocumentSpreadsheetFile(Path file, InputStream source) throws IOException {

-        try (ZipOutputStream out = new ZipOutputStream(new BufferedOutputStream(new FileOutputStream(file)))) {
+        try (ZipOutputStream out = new ZipOutputStream(new BufferedOutputStream(new FileOutputStream(file.toFile())))) {


The last FileOutputStream could be replaced by the Files.newFileOutputStream which directly accepts a path

Siedlerchr · 2017-12-25T16:54:26Z

-    private static void storeOpenOfficeFile(File file, InputStream source) throws Exception {
-        try (ZipOutputStream out = new ZipOutputStream(new BufferedOutputStream(new FileOutputStream(file)))) {
+    private static void storeOpenOfficeFile(Path file, InputStream source) throws Exception {
+        try (ZipOutputStream out = new ZipOutputStream(new BufferedOutputStream(new FileOutputStream(file.toFile())))) {


see as above

Siedlerchr · 2017-12-25T16:56:19Z

 ### Changed

 ### Fixed
+- We fixed the name of an exported file.


Improve changelog entry

Siedlerchr · 2017-12-25T16:58:44Z

+        return importers.stream().filter(importer -> importer.getDescription().equals(extensionFilter.getDescription())).findFirst();
+    }
+
+    public static Optional<Exporter> getExporter(FileChooser.ExtensionFilter extensionFilter, Collection<Exporter> exporters) {


Either you move the whole class to a new "meta" package, or you move the getExporter method to another class
Atm it is still in import and I would not expect an export class here

Moved to gui.util

Siedlerchr · 2017-12-25T16:59:21Z

Some mior things, The only thing we should discuss is the renaming of the FileExtensions to FileType.
For me FileType would include actual information about the type, or a specific class., but in this case it's solely the extension

Siedlerchr · 2017-12-25T17:02:06Z

            return this;
+        }
+
+        public Builder withDefaultExtension(String fileTypeDescription) {


Please add these cases to the unit tests we already have for this class

tobiasdiez · 2017-12-27T16:39:00Z

@Siedlerchr Thanks for the feedback. I now changed the code accordingly.

Why don't you like the name FileType? The class currently contains a description and a list of extensions (e.g., "BibTeX library" with ".bib"). This is a bit for information than just extensions (which would be just a Set<FileExtension>). But I have no strong opinion about the name (the rename was triggered by the code importer.getExtensions().getExtensions()).

lenhard

Overall a nice refactoring. I have some minor code comments, which are partly down to the structure of the code as it has been before.

I don't really have a strong opinion on FileType vs FileExtension.

lenhard · 2018-01-02T14:25:12Z

+                    importMenu.automatedImport(Collections.singletonList(file.toString()));
+                    // Set last working dir for import
+                    Globals.prefs.put(JabRefPreferences.IMPORT_WORKING_DIRECTORY, file.getParent().toString());
+                } catch (Exception ex) {


There's certainly a more specific type of exception you can catch here, isn't it?

lenhard · 2018-01-02T14:27:39Z

+    public String getDescription() {
+        return getFileType().getDescription();
+    }
+}


If I recall our git guidelines correctly, you should add a new line here

There is actually a blank line 58, but github does not show these last empty lines (but it displays a warning symbol if the file actually does not end with an empty line).

lenhard · 2018-01-02T14:28:22Z

+     * @return The string describing available exporters.
+     */
+    public String getExportersAsString(int maxLineLength, int firstLineSubtraction, String linePrefix) {
+        StringBuilder sb = new StringBuilder();


Since you are already touching this code, please also use proper naming, e.g. builder.

lenhard · 2018-01-02T14:32:35Z

@@ -52,7 +52,7 @@ public void performExport(final BibDatabaseContext databaseContext, final String
            } catch (TransformerException | IllegalArgumentException | TransformerFactoryConfigurationError e) {
                throw new Error(e);


Not your code, I know, but isn't it weird to throw an error here? Wouldn't it be better at least scale that down to a RuntimeException?

lenhard · 2018-01-02T14:34:18Z

        this.undoableFieldChanges.addAll(newUndoableFieldChanges);
    }
+
+    public void finalize(Path file) throws SaveException, IOException {


Just a thought: Would it make sense to have this class implement AutoCloseable, so that it could be used in a try-with-resources? This method seems to be built for that use case. Otherwise, it's not guaranteed that the writer will be closed.

Never head about AutoClosable. Thanks for pointing this out. This is a good idea but there are quite a few places where the save session is canceled (i.e. commit or finalized is not called). Moreover, a quick look at the code showed that it is relatively hard to shift the file argument to the constructor of the save session (the creation happens in different classes then the actual commit).

lenhard · 2018-01-02T14:35:05Z

            return;
        }
-        Path outFile = Paths.get(file);
        SaveSession ss = null;


Again, since you are already refactoring this method, please also improve the naming of variables.

lenhard · 2018-01-02T14:36:07Z


    }
-
-    ;


It's really weird what kind of code you still find in JabRef. Great job removing this!

lenhard · 2018-01-02T14:38:32Z

+
    @Test
    public void testExportingEmptyDatabaseYieldsEmptyFile() throws Exception {
        File tmpFile = testFolder.newFile();


Instead of calling toPath below, it would be better if you created a Path object here directly. That applies to the other tests as well.

…ort3576

* upstream/master: Add oaDOI fulltext fetcher (#3581) Refactor export code to fix #3576 (#3578) Fix #3359: Automatically remove colon and apostrophe from key pattern (#3506)

tobiasdiez added 2 commits December 25, 2017 14:56

Refactor exports to fix #3576

ce03e05

Add changelog

8ca327a

tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 25, 2017

Fix build

ab5dfc2

Siedlerchr reviewed Dec 25, 2017

View reviewed changes

Implement feedback and fix tests

d693a62

lenhard reviewed Jan 2, 2018

View reviewed changes

tobiasdiez added 3 commits January 2, 2018 21:25

Merge branch 'master' of https://github.com/JabRef/jabref into fixExp…

d4bd9eb

…ort3576

Include feedback

179b2c7

Fix checkstyle...again...joy!

a46ba2b

tobiasdiez merged commit d381839 into master Jan 2, 2018

tobiasdiez deleted the fixExport3576 branch January 2, 2018 21:13

		@@ -52,7 +52,7 @@ public void performExport(final BibDatabaseContext databaseContext, final String
		} catch (TransformerException \| IllegalArgumentException \| TransformerFactoryConfigurationError e) {
		throw new Error(e);

Uh oh!

Conversation

tobiasdiez commented Dec 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Siedlerchr commented Dec 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobiasdiez commented Dec 27, 2017

Uh oh!

lenhard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobiasdiez Jan 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobiasdiez Jan 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Siedlerchr commented Dec 25, 2017 •

edited

Loading

tobiasdiez Jan 2, 2018 •

edited

Loading

tobiasdiez Jan 2, 2018 •

edited

Loading