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

Fix for issue 2762: Change CSV export to separate all names using semicolon #2793

Merged
merged 4 commits into from
Apr 25, 2017

Conversation

FabianMarcoBauer
Copy link

@FabianMarcoBauer FabianMarcoBauer commented Apr 25, 2017

See #2762
Added a new LayoutFormatter that replaces all occurrences of " and " in the fieldText with "; ".
This formatter is now used for the author and editor fields when exporting to a csv file.

  • Change in CHANGELOG.md described
  • Tests created for changes
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)

125m125 added 2 commits April 25, 2017 14:49
Add LayoutFormatter AuthorAndToSemicolonReplacer that replaces all " and " with "; ". This formatter is used to format authors and editors for the openoffice-csv layout.
@FabianMarcoBauer FabianMarcoBauer changed the title Fix for issue 2762 Fix for issue 2762: Change CSV export to separate all names using semicolon Apr 25, 2017
assertEquals(4, lines.size());
System.out.println(lines.get(3));
assertTrue(lines.get(1).matches("^.*,\"Someone, Van Something\",.*$"));
assertTrue(lines.get(2).matches("^.*,\"von Neumann, John; Smith, John; Black Brown, Peter\",.*$"));
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you can separate this into three test. One for a single author, one for two and the last for N multiples.

Assert.assertEquals("Someone, Van Something", a.format("Someone, Van Something"));

// Two names just one semicolon
Assert.assertEquals("John Smith; Black Brown, Peter", a
Copy link
Member

Choose a reason for hiding this comment

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

Please separate to multiple tests as suggested above. One for each test case.

@stefan-kolb stefan-kolb added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 25, 2017
Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

I only have a few small remarks. Can be merged when these are fixed.

CHANGELOG.md Outdated
@@ -14,6 +14,7 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `#
- Continued to redesign the user interface: this time the editor got a fresh coat of paint:
- The buttons were changed to icons.
- Removed the hidden feature that a double click in the editor inserted the current date.
- All authors and editors are separated using semicolons when exporting to csv.
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 a reference to the fixed issue (see the changelog entries below).

Copy link
Author

Choose a reason for hiding this comment

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

Should I move this entry to the section Fixed or keep it under Changes?

Copy link
Member

Choose a reason for hiding this comment

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

You can leave it there for the moment. Before a new version release we usually check and reorder the changelog

List<String> lines = Files.readAllLines(tmpFile.toPath());
assertEquals(4, lines.size());
System.out.println(lines.get(3));
assertTrue(lines.get(1).matches("^.*,\"Someone, Van Something\",.*$"));
Copy link
Member

Choose a reason for hiding this comment

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

It is probably better to replace the assertTrue with something like assertEquals("mykey1; title1; Someone, Van Something", lines.get(1))

@tobiasdiez
Copy link
Member

Thanks for the quick follow-up and I really like the solution with paramaterized tests (and the fact that you wrote tests in the first place!).

@tobiasdiez tobiasdiez merged commit b8cd026 into JabRef:master Apr 25, 2017
Siedlerchr added a commit that referenced this pull request Apr 29, 2017
* upstream/master: (84 commits)
  Update README.md
  Update CHANGELOG.md
  Fixes #2789 Add Referer to API call (#2794)
  Change some FileDialogs to DialogService (#2767)
  Fix for issue 2762: Change CSV export to separate all names using semicolon (#2793)
  Set eclipse line wrapping to maximum
  Do not log an exception if side pane was not found (#2791)
  Added 'Ink' to the supported FileAnnotationType (required to close #2777)
  Renamed parseFileAnnotationType() to parse()
  Reimplement date editor in JavaFX (#2781)
  Update CONTRIBUTING.md
  Add new author
  Fixes handling of unknown PDAnnotation types.
  Update Checkstyle Version
  fix some more checkstyle warnings
  fix some more checkstyle warnings
  Fix Build failure, hopefully
  Spanish translation (#2773)
  Fixes #2766 If file is not found annotations might be null
  Fix language tests
  ...

# Conflicts:
#	src/main/java/org/jabref/logic/util/io/FileUtil.java
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.

5 participants