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

Refactor writing of bib files #1472

Merged
merged 3 commits into from
Jul 16, 2016

Conversation

tobiasdiez
Copy link
Member

The main aim of this PR is to simplify the implementation of #1451 (sql export) and other exporters.

  • Abstract class BibDatabaseWriter, which controls the generic things of
    writing a database (i.e. sorting entries, applying save actions, some
    basic conversations, ...)
  • Derived class BibtexDatabaseWriter, which only contains logic how to
    actually write to in BibTeX format (should be simple to write similar
    classes for other export formats like ris)
  • Make SaveSession abstract and introduce two implementations which
    write to a temporary file (FileSaveSession) or two a string/buffer
    (StringSaveSession)
  • Move code related to lock files to FileBasedLock

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

@tobiasdiez tobiasdiez added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers dev: code-quality Issues related to code or architecture decisions labels Jun 4, 2016
@tobiasdiez
Copy link
Member Author

Any comments? Can I merge this in (of course after fixing compile errors)?

@tobiasdiez tobiasdiez closed this Jun 20, 2016
@tobiasdiez tobiasdiez reopened this Jun 20, 2016
List<FieldChange> changes = new ArrayList<>();

Optional<FieldFormatterCleanups> saveActions = metaData.getSaveActions();
if (saveActions.isPresent()) {
Copy link
Member

@Siedlerchr Siedlerchr Jun 20, 2016

Choose a reason for hiding this comment

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

Maybe use the lambda version: ifPresent

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, but I find the old-style version more readable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Optional only make sense if one uses ifPresent or map/flatMap. Otherwise, it does not really help that much, from my perspective, as one still requires an if check and a get, making it even more complicated to use instead of a null check and just using it.

@Siedlerchr
Copy link
Member

Overall looks good, just some small remarks from my side 👍

@tobiasdiez tobiasdiez force-pushed the abstractDatabaseWriter branch from 1f5a31d to 892c593 Compare June 26, 2016 17:06
@tobiasdiez
Copy link
Member Author

@Siedlerchr Thanks for you feedback. Incorporated it and rebased.

@tobiasdiez tobiasdiez added this to the v3.6 milestone Jun 28, 2016
* (such as the exportDatabase call), we do not wish to use the
* global preference of saving in standard order.
*/
* We have begun to use getSortedEntries() for both database save operations
Copy link
Member

Choose a reason for hiding this comment

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

indent?

@simonharrer
Copy link
Contributor

@tobiasdiez after rebase and successful tests, this can be merged as well.

- Abstract class BibDatabaseWriter, which controls the generic things of
writing a database (i.e. sorting entries, applying save actions, some
basic conversations, ...)
- Derived class BibtexDatabaseWriter, which only contains logic how to
actually write to in BibTeX format (should be simple to write similar
classes for other export formats like ris)
- Make SaveSession abstract and introduce two implementations which
write to a temporary file (FileSaveSession) or two a string/buffer
(StringSaveSession)
- Move code related to lock files to FileBasedLock
@tobiasdiez tobiasdiez force-pushed the abstractDatabaseWriter branch from a00f7ac to 013c27c Compare July 16, 2016 08:42
@tobiasdiez tobiasdiez merged commit 622a0da into JabRef:master Jul 16, 2016
@tobiasdiez tobiasdiez deleted the abstractDatabaseWriter branch July 16, 2016 08:51
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
dev: code-quality Issues related to code or architecture decisions 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.

4 participants