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

Feature java version check again #3428

Merged
merged 4 commits into from
Nov 13, 2017
Merged

Conversation

halirutan
Copy link
Collaborator

OK, I added some comment about the Java 9 check and why we should leave it in for the moment. The rest is unchanged but updated to the latest master.

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

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.

Code looks good to me. But for the next time: please use parametrized tests (for junit5 but I'm not sure we have already updated, there is also something similar for junit4).

@halirutan
Copy link
Collaborator Author

@tobiasdiez I'm not exactly sure how this would have been possible here. For some sub-test sure, but would it have been possible to make nested loops where I test all java8 versions against all java9 version when I provide them as parameter test?

@tobiasdiez
Copy link
Member

Yes, I think that is possible. The junit 5 documentation contains the code

@ParameterizedTest
@MethodSource("stringAndIntProvider")
void testWithMultiArgMethodSource(String first, int second) {
    assertNotNull(first);
    assertNotEquals(0, second);
}

static Stream<Arguments> stringAndIntProvider() {
    return Stream.of(Arguments.of("foo", 1), Arguments.of("bar", 2));
}

By changing stringAndIntProvider you can generate every possible combination of java 8 and java 9 version strings. Anyway, no need to change it for this PR. Was just a remark for the next time.

@halirutan
Copy link
Collaborator Author

Can this be merged? If yes, would some of the veterans do this?

@Siedlerchr
Copy link
Member

@halirutan If you are ready, add the label "ready for review". From my point of view it's good now.

@halirutan halirutan added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Nov 13, 2017
@tobiasdiez tobiasdiez merged commit c1d425a into master Nov 13, 2017
@tobiasdiez tobiasdiez deleted the feature-JavaVersionCheck branch November 13, 2017 12:34
Siedlerchr added a commit that referenced this pull request Nov 18, 2017
* upstream/master: (23 commits)
  Feature java version check again (#3428)
  Fix test for quoted lang messages (#3424)
  Update gradle from 4.3 to 4.3.1
  Fix #3411: ordering of fields in customized entry types works again (#3422)
  Backport of syncLang to python2 (#3420)
  Remove Versioneye badge
  Fix some error prone warnings
  Fix for issue #2721 append to a field (#3395)
  Fix travis - hopefully
  Remove 3.x changelog (#3250)
  Try to use hint of https://github.com/TheBoegl/shadow-log4j-transformer#usage-as-library
  Try to enable LGTM
  Update guava from 23.2 -> 23.3 (#3409)
  Update wiremock from 2.8.0 -> 2.10.1
  Move groups field from others to general (#3407)
  Fix checkstyle issues to repair build
  Fix #3046: No longer allow duplicate fields in customized entry types (#3405)
  Strip invalid prolog when loading CitationStyles (#3404)
  Integrity check "Abbreviation Detection" detects abbreviated names for journals and booktitles based on the internal list instead of only looking for "." signs. (#3362)
  Update gradle from 4.2.1 to 4.3
  ...
Siedlerchr added a commit that referenced this pull request Nov 19, 2017
* upstream/master: (30 commits)
  Add Hint for checking master version (#3439)
  Replace LinkedFiles backslashes with forward slashes (#3403)
  fix isbn result from chimbori (#3442)
  Feature java version check again (#3428)
  Fix test for quoted lang messages (#3424)
  Update gradle from 4.3 to 4.3.1
  Fix #3411: ordering of fields in customized entry types works again (#3422)
  Backport of syncLang to python2 (#3420)
  Remove Versioneye badge
  Fix some error prone warnings
  Fix for issue #2721 append to a field (#3395)
  Fix travis - hopefully
  Remove 3.x changelog (#3250)
  Try to use hint of https://github.com/TheBoegl/shadow-log4j-transformer#usage-as-library
  Try to enable LGTM
  Update guava from 23.2 -> 23.3 (#3409)
  Update wiremock from 2.8.0 -> 2.10.1
  Move groups field from others to general (#3407)
  Fix checkstyle issues to repair build
  Fix #3046: No longer allow duplicate fields in customized entry types (#3405)
  ...
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