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 errorprone #3151

Merged
merged 7 commits into from
Aug 24, 2017
Merged

Fix errorprone #3151

merged 7 commits into from
Aug 24, 2017

Conversation

Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Aug 23, 2017

Fix #3151

  • Main problem: comments after a line are always assumed to be somehow named parameter arguments

  • Some Tests were not run (missing test annotation)

  • Some complex boolean expressions aka (0==1) where false should be used

  • 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?

@Siedlerchr Siedlerchr requested a review from koppor August 23, 2017 15:45
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 23, 2017
Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Besiders minor comments: LGTM

@@ -20,7 +20,7 @@ public void testFixAuthorNatbib() {
// Is not cached!
Assert.assertTrue(AuthorList
.fixAuthorNatbib("John von Neumann and John Smith and Black Brown, Peter").equals(AuthorList
.fixAuthorNatbib("John von Neumann" + (0 == 1 ? "" : " and ")
.fixAuthorNatbib("John von Neumann" + (false ? "" : " and ")
Copy link
Member

Choose a reason for hiding this comment

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

Why not removing the condition completly? Just "John von Neumann and John Smith and Black Brown, Peter"?

Similar comments for the other false checks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

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.

Looks good, I'll merge.

I wonder why these strange 0 == 1 expressions were put into the code. I cannot come up with a single motivation for that.

@lenhard lenhard merged commit 0d7dbb9 into master Aug 24, 2017
@lenhard lenhard mentioned this pull request Aug 24, 2017
Siedlerchr added a commit that referenced this pull request Aug 24, 2017
* upstream/master:
  Fix errorprone (#3151)
  Fix display of percentage symbol in entry preview (#3149)
@stefan-kolb stefan-kolb deleted the fixErrorprone branch August 25, 2017 11:27
Siedlerchr added a commit that referenced this pull request Aug 27, 2017
* upstream/master:
  Add preference to disable validation in the entry editor by default (#3154)
  Fix freezing on browse in protected terms dialog when adding from entry editor (#3158)
  Close entry editor when the shown entry is removed externally (#3156)
  .jar are not ignored any more (#3155)
  Increase size of file and keywords editors (#3141)
  Make Replace Strings work in bibtexkeyfield
  Fix errorprone (#3151)
  Fix display of percentage symbol in entry preview (#3149)
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.

3 participants