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

Don't trim when migrating review field #3761

Merged
merged 5 commits into from
Feb 21, 2018
Merged

Don't trim when migrating review field #3761

merged 5 commits into from
Feb 21, 2018

Conversation

LinusDietz
Copy link
Member

fixes koppor#311

@LinusDietz LinusDietz added bug Confirmed bugs or reports that are very likely to be bugs status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Feb 21, 2018
@koppor
Copy link
Member

koppor commented Feb 21, 2018

This does not help, because IN THE MIDDLE the newlines are deleted.

@Article{AHP1996,
  author    = {R. Alur and G. J. Holzmann and D. Peled},
  title     = {An analyzer for Message Sequence Charts},
  journal   = {Software - Concepts and Tools},
  year      = {1996},
  pages     = {70–77},
  volume    = {17},
  number    = {2},
  keywords  = {TODO, race conditions},
  owner     = {koppor},
  review    = {TSE cites it as THE definition of "race conditions" (page 27)

In the literature about MSCs, the possibility that messages may be received in a different order than the one specified is usually called a race condition [2].},
  timestamp = {2011.04.17},
}

@@ -48,7 +48,7 @@ public void performConflictingMigration(ParserResult parserResult) {
private String mergeCommentFieldIfPresent(BibEntry entry, String review) {
if (entry.getField(FieldName.COMMENT).isPresent()) {
LOGGER.info(String.format("Both Comment and Review fields are present in %s! Merging them into the comment field.", entry.getAuthorTitleYear(150)));
return String.format("%s\n%s:\n%s", entry.getField(FieldName.COMMENT).get().trim(), Localization.lang("Review"), review.trim());
return String.format("%s\n%s:\n%s", entry.getField(FieldName.COMMENT).get(), Localization.lang("Review"), review);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe, %s does something magic? Or does the get() do magic?

Copy link
Member Author

Choose a reason for hiding this comment

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

wow. well I'll write a unit test for that and then we'll see.

Copy link
Member

Choose a reason for hiding this comment

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

I always forget the specifc string arguments formatter, too
%$1s etc- Maybe that helps?
An argument index is specified as a number ending with a “$” after the “%” and selects the specified argument in the argument list.
https://dzone.com/articles/java-string-format-examples

Copy link
Member

Choose a reason for hiding this comment

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

No, we just forgot that COMMENT is a mutli line field. We decided two or three years ago that JabRef rewraps all field values to have a good looking bibtex file. We make exceptions for predefined multi line fields. I am working on an update.

@koppor
Copy link
Member

koppor commented Feb 21, 2018

I reverted the revert of trimming. I think, that is a good thing to do!

@LinusDietz
Copy link
Member Author

I'm approving this as well. Whoever merges, don't squash so @koppor gets credit as well!

@@ -28,6 +28,7 @@ public FieldContentParser(FieldContentParserPreferences prefs) {
multiLineFields = new HashSet<>();
// the following two are also coded in org.jabref.logic.bibtex.LatexFieldFormatter.format(String, String)
multiLineFields.add(FieldName.ABSTRACT);
Copy link
Member

Choose a reason for hiding this comment

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

Please extract these fields that should be treated as multilines to InternalBibtexFields (or a better location). Moreover, a few places still work with

public static final String REVIEW = "review";
instead of comments. This should be changed too.

@koppor
Copy link
Member

koppor commented Feb 21, 2018

I treat this at hotfix. I copied the comment by @tobiasdiez to koppor#314

@koppor koppor merged commit 58c2c7a into master Feb 21, 2018
@koppor koppor deleted the review-migration-trim branch February 21, 2018 21:24
@koppor
Copy link
Member

koppor commented Feb 21, 2018

Siedlerchr added a commit that referenced this pull request Feb 24, 2018
* upstream/master: (94 commits)
  Add missing localization for Any file
  Refactor dublin core utility (#3756)
  Add Localization
  Update Architecture Tests to catch static imports (#3766)
  Added <any file type> to the Import File Filter Dialog.
  Don't trim when migrating review field (#3761)
  Reorder again
  Rename confirmation into "Merge fields"
  Fix logic
  Reorder checklist in PR template and add "good commit message"
  Replace x11 by unity7
  Include desktop, desktop-legacy, wayland in snapcraft.yaml
  Improve Dublin Core (#3710)
  Incorporate suggestions by @Siedlerchr
  Update JUnit from 5.1.0-M2 -> 5.1.0
  Update Mockito from 2.13.0 -> 2.15.0
  Update wiremock from 2.14.0 -> 2.15.0
  Fix exceptions for jacoco
  update gradle
  Add link to contribute.jabref.org (#3748)
  ...

# Conflicts:
#	src/main/java/org/jabref/gui/JabRefFrame.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bugs or reports that are very likely to be bugs 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.

Review to Comment migration destroys newlines
4 participants