Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Unify OpenDatabaseAction and Importer #1165

Closed
tobiasdiez opened this issue Apr 11, 2016 · 4 comments
Closed

Unify OpenDatabaseAction and Importer #1165

tobiasdiez opened this issue Apr 11, 2016 · 4 comments
Labels
component: cleanup-ops dev: code-quality Issues related to code or architecture decisions [outdated] type: question

Comments

@tobiasdiez
Copy link
Member

I had a look at how to fix #1080 and #1153 and recognized that there is quite some overlap between the Importer package and some code in GUI (in particular, the class OpenDatabaseAction).

I propose to unify the code used to open or import a database.

  • Change List<BibEntry> importEntries(InputStream in, OutputPrinter status) in ImportFormat to a method ParserResult import(InputStream in) which might also return some metadata. The returned ParserResult contains potential error messages (which in my opinion is cleaner than letting JabRefFrame implement OutputPrinter).
  • Add default method import(Path file, Charset defaultEncoding) to the importer interface which opens the file and passes the stream to the other import method.
  • The BibTexImporter can overwrite import(Path file, Charset defaultEncoding) and use the logic from OpenDatabaseAction.loadDatabase to determine the correct encoding.
  • Change OpenDatabaseAction to operate against the (BibTex)importer.
  • At this point the importFromFile methods in ImportFormatReader can be removed.
  • Remove isRecognizedFormat: this method is only used directly before the import, so ImportFormat.import can just return a corresponding warning message.
  • Remove get/set isCustomImporter: is not really used (just for sorting and I think a completely alphabetic display is more logical)
  • getExtension and getDescription is not implemented for a lot of importer?!
  • Changed: Empty entries are no longer imported (thus a few test had to be changed)

What do you think about the proposed changes?

@tobiasdiez tobiasdiez added [outdated] type: question component: cleanup-ops dev: code-quality Issues related to code or architecture decisions labels Apr 11, 2016
@lenhard
Copy link
Member

lenhard commented Apr 11, 2016

Sounds fine to me and I think you can go ahead with a PR. There is no reason to duplicate the BibTeX file reading logic.

Just be prepared to run into unforseen complications that thwart your plan (as always in JabRef) :)

@oscargus
Copy link
Contributor

Also, unifying append and import to existing would be nice, see #262

@oscargus
Copy link
Contributor

Regarding isRecognizedFormat, I think it makes sense to keep. The whole point is to be able to guess format without doing an actual import. Given that it is implemented correctly, there should be a significant complexity reduction compared to actually trying to import in each and every format.

@tobiasdiez
Copy link
Member Author

Thanks for the feedback. The rest can be discussed in the PR #1207

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
component: cleanup-ops dev: code-quality Issues related to code or architecture decisions [outdated] type: question
Projects
None yet
Development

No branches or pull requests

3 participants