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

Restructure importers #1207

Merged
merged 8 commits into from
May 24, 2016
Merged

Restructure importers #1207

merged 8 commits into from
May 24, 2016

Conversation

tobiasdiez
Copy link
Member

The aim of this PR is to reduce the overlap between the Importer package and some code in GUI (in particular, the class OpenDatabaseAction).
Moreover, #1080 and #1153 are fixed.

In detail, the following things were changed:

  • The abstract ImportFormat class is almost completely rewritten. Please have a look at this class. Almost all other changes are a direct consequence of the edits made to the ImportFormat class. The biggest change is that the import method returns a ParserResult instead of a list of entries (in this way, metadata can also be imported and error messages are communicated more effectively instead of throwing IOExceptions). Moreover, the import method operates on a reader instead of an input stream in order to reduce some common code in all importers.
  • Add method importDatabase(Path file, Charset encoding) to the importer interface which opens the file and passes the stream to the other import method.
  • The BibTexImporter overwrites this new import method and uses the logic from OpenDatabaseAction.loadDatabase to determine the encoding of the bib file.
  • Change OpenDatabaseAction to operate against the BibTex importer.
  • Remove get/set isCustomImporter: was not really used (just for sorting - now everything is alphabetical)
  • Empty entries are no longer imported (thus a few test had to be changed)

Remark: The methods getExtension and getDescription are not implemented by almost any importer. This should be done. Maybe this is something for the stupro?

(Will add the changelog entry and add a few tests later)

  • Change in CHANGELOG.md described
  • Tests created for changes
  • Screenshots added (for bigger UI changes)

@tobiasdiez tobiasdiez added cleanup-ops status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Apr 14, 2016
@simonharrer simonharrer added this to the v3.4 milestone Apr 14, 2016
@tobiasdiez
Copy link
Member Author

Any comments or can I rebase and merge?

@Override
public String getCLIId() {
return "ris";
public List<String> getExtensions() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why return null here (and in similar places)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since I have no idea for which extensions these importers support.

See above:

Remark: The methods getExtension and getDescription are not implemented by almost any importer. This should be done. Maybe this is something for the stupro?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, OK. Tired... Somehow, the questions was also, why not an empty list?

@oscargus
Copy link
Contributor

I do not fully understand why the getCLIId method is removed? How is the importer selected from CLI now?

Also, since there is a method setCliId, maybe that should change name as well (as it appears as getCLIId is sometimes replaces with getId, somehow, it seems like getCLIId is a better name, but no strong opinions).

Many changes are related to imports and replacing extension strings with a list of extension strings, right?

@oscargus
Copy link
Contributor

Now I think I understand why. However, I think that getCLIId and getFormatName in general should return different things...

@oscargus
Copy link
Contributor

Well, maybe I do not understand the code fully (again, tired), but what I was worried about with getId doesn't seem to the case, so to me this looks good!

@tobiasdiez
Copy link
Member Author

tobiasdiez commented Apr 29, 2016

There is a method getId (previously getCLIID) in ImportFormat which constructs the id based on the name of the formatter. Thus some of the hand-coded ids in the specific importers could be removed.

I couldn't find the method setCLIID. Where does it exists?

@oscargus
Copy link
Contributor

I saw "REPEC New Economic Papers (NEP)" and then that getId() used that string which caused one comment, but then I saw the tests and realized that one wasn't supposed to type something like that. :-) Now, I also know why.

👍

@simonharrer
Copy link
Contributor

Waiting for the last #547 to be merged - then, this PR can clean up all the importer code uniformly.

This was referenced May 18, 2016
@simonharrer
Copy link
Contributor

This is unblocked now. Go 4 it. :)

# Conflicts:
#	src/main/java/net/sf/jabref/Globals.java
#	src/main/java/net/sf/jabref/JabRef.java
#	src/main/java/net/sf/jabref/exporter/SaveDatabaseAction.java
#	src/main/java/net/sf/jabref/external/ExternalFileTypeEntryEditor.java
#	src/main/java/net/sf/jabref/external/MoveFileAction.java
#	src/main/java/net/sf/jabref/gui/BasePanel.java
#	src/main/java/net/sf/jabref/gui/FileDialogs.java
#	src/main/java/net/sf/jabref/gui/FileListEntryEditor.java
#	src/main/java/net/sf/jabref/gui/actions/BrowseAction.java
#	src/main/java/net/sf/jabref/gui/journals/ManageJournalsPanel.java
#	src/main/java/net/sf/jabref/gui/plaintextimport/TextInputDialog.java
#	src/main/java/net/sf/jabref/importer/ImportFormatReader.java
#	src/main/java/net/sf/jabref/importer/OpenDatabaseAction.java
#	src/main/java/net/sf/jabref/importer/fetcher/DOItoBibTeXFetcher.java
#	src/main/java/net/sf/jabref/importer/fetcher/MedlineFetcher.java
#	src/main/java/net/sf/jabref/importer/fileformat/BiblioscapeImporter.java
#	src/main/java/net/sf/jabref/importer/fileformat/FreeCiteImporter.java
#	src/main/java/net/sf/jabref/importer/fileformat/InspecImporter.java
#	src/main/java/net/sf/jabref/importer/fileformat/IsiImporter.java
#	src/main/java/net/sf/jabref/importer/fileformat/MedlineImporter.java
#	src/main/java/net/sf/jabref/importer/fileformat/MedlinePlainImporter.java
#	src/main/java/net/sf/jabref/importer/fileformat/MsBibImporter.java
#	src/main/java/net/sf/jabref/importer/fileformat/OvidImporter.java
#	src/main/java/net/sf/jabref/importer/fileformat/PdfContentImporter.java
#	src/main/java/net/sf/jabref/importer/fileformat/PdfXmpImporter.java
#	src/main/java/net/sf/jabref/importer/fileformat/RepecNepImporter.java
#	src/main/java/net/sf/jabref/importer/fileformat/RisImporter.java
#	src/main/java/net/sf/jabref/importer/fileformat/SilverPlatterImporter.java
#	src/main/java/net/sf/jabref/logic/msbib/MSBibDatabase.java
#	src/main/java/net/sf/jabref/logic/xmp/XMPUtil.java
#	src/main/java/net/sf/jabref/pdfimport/PdfImporter.java
#	src/test/java/net/sf/jabref/exporter/BibDatabaseWriterTest.java
#	src/test/java/net/sf/jabref/importer/ImportFormatReaderIntegrationTest.java
#	src/test/java/net/sf/jabref/importer/fileformat/BiblioscapeImporterTest.java
#	src/test/java/net/sf/jabref/importer/fileformat/BiblioscapeImporterTestFiles.java
#	src/test/java/net/sf/jabref/importer/fileformat/BiblioscapeImporterTestTypes.java
#	src/test/java/net/sf/jabref/importer/fileformat/CopacImporterTest.java
#	src/test/java/net/sf/jabref/importer/fileformat/EndnoteImporterTest.java
#	src/test/java/net/sf/jabref/importer/fileformat/InspecImportTest.java
#	src/test/java/net/sf/jabref/importer/fileformat/IsiImporterTest.java
#	src/test/java/net/sf/jabref/importer/fileformat/MedlinePlainImporterTest.java
#	src/test/java/net/sf/jabref/importer/fileformat/MsBibImporterTestfiles.java
#	src/test/java/net/sf/jabref/importer/fileformat/OvidImporterTest.java
#	src/test/java/net/sf/jabref/importer/fileformat/PdfContentImporterTestFiles.java
#	src/test/java/net/sf/jabref/importer/fileformat/PdfXmpImporterTest.java
#	src/test/java/net/sf/jabref/importer/fileformat/RISImporterTestFiles.java
#	src/test/java/net/sf/jabref/importer/fileformat/RepecNepImporterTest.java
#	src/test/java/net/sf/jabref/importer/fileformat/SilverPlatterImporterTest.java
#	src/test/java/net/sf/jabref/logic/openoffice/OOBibStyleTest.java
@koppor koppor added the type: code-quality Issues related to code or architecture decisions label Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup-ops status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers type: code-quality Issues related to code or architecture decisions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants