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.

Sign up for GitHub

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

Jump to bottom

[WIP] Add "Convert to BibTeX format" cleanup #3541

Merged
merged 5 commits into from
Jan 18, 2018
Merged

[WIP] Add "Convert to BibTeX format" cleanup #3541

merged 5 commits into from
Jan 18, 2018

Conversation

tobiasdiez
Copy link
Member

As wished in the forum and required for #1018.

  • 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 15, 2017
@LinusDietz
Copy link
Member

What about adding some round-trip integration tests where a bibtex entry is converted into biblatex and back asserting that nothing has changed?
This would give us a bit more confidence, that all of this is working fine. You could start with the example from the forums http://discourse.jabref.org/t/convert-back-from-biblatex-to-bib/955/6

@koppor koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 22, 2017
@koppor koppor changed the title Add "Convert to BibTeX format" cleanup [WIP] Add "Convert to BibTeX format" cleanup Dec 22, 2017
@tobiasdiez tobiasdiez changed the title [WIP] Add "Convert to BibTeX format" cleanup Add "Convert to BibTeX format" cleanup Dec 23, 2017
@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 23, 2017
@tobiasdiez
Copy link
Member Author

@LinusDietz good suggestion. I now added round-trip tests in both directions.

@tobiasdiez tobiasdiez force-pushed the implementBiblatexToBibtexCleanup branch from 91e5fd3 to c700bfc Compare December 23, 2017 14:06
lenhard
lenhard reviewed Jan 12, 2018
View reviewed changes
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.

The code looks fine and from playing around in the UI things seem to work.

Could you perhaps do a small usability improvement? It is possible to select both cleanup operations (to bibtex and to biblatex) in the UI. That is a bit weird. Could you make these options modal to each other?

@koppor koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jan 18, 2018
@koppor koppor changed the title Add "Convert to BibTeX format" cleanup [WIP] Add "Convert to BibTeX format" cleanup Jan 18, 2018
@tobiasdiez tobiasdiez merged commit beb6ee0 into master Jan 18, 2018
@tobiasdiez
Copy link
Member Author

@lenhard Thanks for the review and the suggestion to improve the UI. I now made the checkboxes modal.

I'll merge this PR now since it has 1.5 positive reviews and was on "ready-for-review" for almost a month.

@tobiasdiez tobiasdiez deleted the implementBiblatexToBibtexCleanup branch January 18, 2018 15:38
Siedlerchr added a commit that referenced this pull request Jan 24, 2018
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into selectFilesDlg
3f85f65 
* upstream/master: (46 commits)
  Replace openoffice jars with libreoffice  jars (#3662)
  Export no empty lines in RIS format (#3661)
  Remove apache commons logging in favor of slf4j + log4j for JAVA 9 (#3653)
  fix isbn fetcher test as Joshua Bloch has published a new revivsion of Effetive Java convert to junit5 jupiter
  Extend RIS import with multiple fields (#3642)
  Fix ICAR fetcher test which resulted in build failure (#3654)
  New translations JabRef_en.properties (French) (#3650)
  Add link to MADR and fix typo
  [WIP] Add "Convert to BibTeX format" cleanup (#3541)
  Fix typo
  Fix typo
  Quickfix to get build running on all platforms (#3638)
  New translations JabRef_en.properties (French)
  New translations JabRef_en.properties (Vietnamese)
  New translations JabRef_en.properties (Chinese Simplified)
  New translations JabRef_en.properties (Dutch)
  New translations JabRef_en.properties (French)
  New translations JabRef_en.properties (German)
  New translations JabRef_en.properties (Greek)
  New translations JabRef_en.properties (Indonesian)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lenhard lenhard lenhard left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tobiasdiez @LinusDietz @lenhard @koppor