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

Refactor export code to fix #3576 #3578

Merged
merged 7 commits into from
Jan 2, 2018
Merged

Refactor export code to fix #3576 #3578

merged 7 commits into from
Jan 2, 2018

Conversation

tobiasdiez
Copy link
Member

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?

@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 25, 2017
*/
class ModsExportFormat extends ExportFormat {
class ModsExporter extends TemplateExporter {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

*/
class MSBibExportFormat extends ExportFormat {
class MSBibExporter extends TemplateExporter {
Copy link
Member

Choose a reason for hiding this comment

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

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


try (ZipOutputStream out = new ZipOutputStream(new BufferedOutputStream(new FileOutputStream(file)))) {
try (ZipOutputStream out = new ZipOutputStream(new BufferedOutputStream(new FileOutputStream(file.toFile())))) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

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())))) {
Copy link
Member

Choose a reason for hiding this comment

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

see as above

CHANGELOG.md Outdated
@@ -14,6 +14,7 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `#
### Changed

### Fixed
- We fixed the name of an exported file.
Copy link
Member

Choose a reason for hiding this comment

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

Improve changelog entry

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

return importers.stream().filter(importer -> importer.getDescription().equals(extensionFilter.getDescription())).findFirst();
}

public static Optional<Exporter> getExporter(FileChooser.ExtensionFilter extensionFilter, Collection<Exporter> exporters) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to gui.util

@Siedlerchr
Copy link
Member

Siedlerchr commented Dec 25, 2017

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

defaultExtension = toFilter(extension);
return this;
}

public Builder withInitialFileName(String initialFileName) {
this.initialFileName = initialFileName;
return this;
}

public Builder withDefaultExtension(String fileTypeDescription) {
Copy link
Member

Choose a reason for hiding this comment

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

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

@tobiasdiez
Copy link
Member Author

@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()).

Copy link
Member

@lenhard lenhard left a comment

Choose a reason for hiding this comment

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

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.

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) {
Copy link
Member

Choose a reason for hiding this comment

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

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

public String getDescription() {
return getFileType().getDescription();
}
}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@tobiasdiez tobiasdiez Jan 2, 2018

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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?

@@ -49,4 +55,14 @@ public void commit(String path) throws SaveException {
public void addFieldChanges(List<FieldChange> newUndoableFieldChanges) {
this.undoableFieldChanges.addAll(newUndoableFieldChanges);
}

public void finalize(Path file) throws SaveException, IOException {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@tobiasdiez tobiasdiez Jan 2, 2018

Choose a reason for hiding this comment

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

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

Objects.requireNonNull(databaseContext);
Objects.requireNonNull(entries);
if (entries.isEmpty()) { // Do not export if no entries to export -- avoids exports with only template text
return;
}
Path outFile = Paths.get(file);
SaveSession ss = null;
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -255,7 +255,4 @@ public void characters(char ch[], int start, int length) throws SAXException {
}

}

;
Copy link
Member

Choose a reason for hiding this comment

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

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

}
return result;
}

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

Choose a reason for hiding this comment

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

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.

@tobiasdiez tobiasdiez merged commit d381839 into master Jan 2, 2018
@tobiasdiez tobiasdiez deleted the fixExport3576 branch January 2, 2018 21:13
Siedlerchr added a commit that referenced this pull request Jan 2, 2018
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

missing dot in files exported
3 participants