Modified configuration in order to work with java 11 #25

fborriello · 2019-01-24T08:07:20Z

Modified cobertura-maven-plugin configuration in order to be not inherited by submodules.
Updated jacoco-maven-plugin version to 0.8.2 (was 0.8.1)..
Updated maven-compiler-plugin version to 3.8.0 (was 3.7.0)..
Added maven-compiler-plugin option in order to add constructor parameter's names to compiled classes.
Updated jdk version to 1.8 (was 1.7)..

…inherited by submodules. - Updated `jacoco-maven-plugin` version to 0.8.2 (was 0.8.1).. - Updated `maven-compiler-plugin` version to 3.8.0 (was 3.7.0).. - Added `maven-compiler-plugin` option in order to add constructor parameter's names to compiled classes. - Updated `jdk` version to 1.8 (was 1.7)..

massdosage · 2019-01-24T09:36:19Z

CHANGELOG.md

@@ -4,6 +4,14 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](http://keepachangelog.com/)
 and this project adheres to [Semantic Versioning](http://semver.org/).

+## [2.3.6] - 2019-01-24


We don't fill in the number or date until we know for sure what these are going to be, so you can set these both to "TBD" for now.

Done, thanks

massdosage · 2019-01-24T09:37:30Z

CHANGELOG.md

+- Updated `jacoco-maven-plugin` version to 0.8.2 (was 0.8.1)..
+- Updated `maven-compiler-plugin` version to 3.8.0 (was 3.7.0)..
+- Added `maven-compiler-plugin` option in order to add constructor parameter's names to compiled classes.
+- Updated `jdk` version to 1.8 (was 1.7)..


This is a backwards incompatible change for some downstream users so we should go with a major version bump of this pom to 3.0.0. Can you also put this line up at the top as the first line so people are more likely to notice it as this is the biggest change.

Done, thanks

massdosage · 2019-01-24T09:37:55Z

CHANGELOG.md

+## [2.3.6] - 2019-01-24
+### Changed
+- Modified `cobertura-maven-plugin` configuration in order to be not inherited by submodules.
+- Updated `jacoco-maven-plugin` version to 0.8.2 (was 0.8.1)..


Remove the extra period at the end, same for next line and line 13.

massdosage · 2019-01-24T09:38:31Z

pom.xml

@@ -114,6 +114,7 @@
          <target>${jdk.version}</target>
          <encoding>${project.build.sourceEncoding}</encoding>
          <showWarnings>true</showWarnings>
+          <parameters>true</parameters>


What does this do and why is it needed?

By default, the java compiler substitutes the constructor's parameter's names with arg0, arg1, etc. With this option, the parameter's names are kept.
For instance, given the following constructor:

public Foo(final String name, final BigInteger id) { this.name = name; this.id = id; }

without that option will look like:

public Foo(final String arg0, final BigInteger arg1) { this.name = arg0; this.id = arg1; }

witht that option enabled will look like:

public Foo(final String name, final BigInteger id) { this.name = name; this.id = id; }

I've added it to parent pom as, this helps a lot the class injection from various bean mapping frameworks available on the web: such as BULL, Jackson, etc.
So coming back to your question, this is not strictly related to the Cobertura issue fix

OK, I didn't know that but it sounds like a useful change.

massdosage · 2019-01-24T09:41:18Z

pom.xml

@@ -135,6 +136,7 @@
        <groupId>org.codehaus.mojo</groupId>
        <artifactId>cobertura-maven-plugin</artifactId>
        <version>${cobertura.maven.plugin.version}</version>
+        <inherited>false</inherited>


I guess this just means this plugin isn't available for use in child projects right? So then it would only apply to this parent module where it's irrelevant since this is a pom with no Java code or tests to report coverage on. So I think we should either just remove it entirely (which I'm fine with) or we look into using to control how its used downstream.

you're right, so as it's fine with you, I removed it and added this change to the changelog.

massdosage · 2019-01-24T10:11:42Z

pom.xml

@@ -114,6 +114,7 @@
          <target>${jdk.version}</target>
          <encoding>${project.build.sourceEncoding}</encoding>
          <showWarnings>true</showWarnings>
+          <parameters>true</parameters>


OK, I didn't know that but it sounds like a useful change.

CHANGELOG.md

massdosage

Actually, we still need cobertura available as a plugin, just not a reporting plugin

massdosage · 2019-01-24T11:17:43Z

pom.xml

@@ -39,17 +39,15 @@

  <properties>
    <buildnumber.maven.plugin.version>1.4</buildnumber.maven.plugin.version>
-    <cobertura.version>2.1.1</cobertura.version>


Can you put this back, I mean to just remove it from the reporting section. I think having the normal plugin in the pluginManagement section is fine, if you don't want to use it downstream then you don't have to. If we remove it entirely from here we'll have to change quite a few downstream projects.

done, thanks

massdosage · 2019-01-24T11:18:00Z

pom.xml

@@ -131,27 +130,6 @@
          </archive>
        </configuration>
      </plugin>
-      <plugin>


Put this one back.

massdosage · 2019-01-24T11:18:49Z

pom.xml

@@ -517,20 +495,6 @@
          <encoding>${project.build.sourceEncoding}</encoding>
        </configuration>
      </plugin>
-      <plugin>


Keep this one removed as this was causing the issue with mvn:site and we don't use this in any downstream projects so it won't have any impact.

done, thanks

…lo/hotels-oss-parent into feature/make-cobertura-optional

fborriello

Changed

AnanaMJ · 2019-01-24T11:44:39Z

CHANGELOG.md

+- Removed `cobertura-maven-plugin`, from reporting sections, as it's not compatible with JDK versions greater than 1.8.
+- Updated `maven-compiler-plugin` version to 3.8.0 (was 3.7.0).
+- Updated `jacoco-maven-plugin` version to 0.8.2 (was 0.8.1).
+- Added `maven-compiler-plugin` option in order to add constructor parameter's names to compiled classes.


"constructor parameter's" -> constructor parameters' (if you mean more than one constructor parameter)

done, thanks

AnanaMJ · 2019-01-24T12:07:59Z

Does mvn site work locally for you? For me it doesn't, but maybe I set up some things incorrectly.

fborriello · 2019-01-24T13:07:18Z

Does mvn site work locally for you? For me it doesn't, but maybe I set up some things incorrectly.

yes it does for me

massdosage · 2019-01-24T13:31:48Z

OK, so I tested this locally and can confirm that it works for me.

massdosage requested a review from a team January 24, 2019 09:35

massdosage suggested changes Jan 24, 2019

View reviewed changes

Modified changelog as per PR

03581d0

massdosage reviewed Jan 24, 2019

View reviewed changes

fborriello and others added 3 commits January 24, 2019 11:57

Updated changelog and removed cobertura plugin

6293507

small tidy up

badc9f9

update version number

1f6ade2

massdosage approved these changes Jan 24, 2019

View reviewed changes

massdosage suggested changes Jan 24, 2019

View reviewed changes

massdosage reviewed Jan 24, 2019

View reviewed changes

fborriello added 3 commits January 24, 2019 12:33

restored as per pr

6777fc3

Merge branch 'feature/make-cobertura-optional' of github.com:fborriel…

d5e67a8

…lo/hotels-oss-parent into feature/make-cobertura-optional

restored cobertura plugin in the build section

0e91e3c

fborriello commented Jan 24, 2019

View reviewed changes

massdosage approved these changes Jan 24, 2019

View reviewed changes

AnanaMJ reviewed Jan 24, 2019

View reviewed changes

fborriello closed this Jan 24, 2019

minor: typo fix into changelog

26b6f6b

massdosage reopened this Jan 24, 2019

AnanaMJ approved these changes Jan 24, 2019

View reviewed changes

massdosage merged commit 73467b2 into HotelsDotCom:master Jan 24, 2019

fborriello deleted the feature/make-cobertura-optional branch June 25, 2019 08:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modified configuration in order to work with java 11 #25

Modified configuration in order to work with java 11 #25

fborriello commented Jan 24, 2019

massdosage Jan 24, 2019

fborriello Jan 24, 2019

massdosage Jan 24, 2019

fborriello Jan 24, 2019

massdosage Jan 24, 2019

fborriello Jan 24, 2019

massdosage Jan 24, 2019

fborriello Jan 24, 2019

massdosage Jan 24, 2019

massdosage Jan 24, 2019

fborriello Jan 24, 2019

massdosage Jan 24, 2019

massdosage left a comment

massdosage Jan 24, 2019

fborriello Jan 24, 2019

massdosage Jan 24, 2019

fborriello Jan 24, 2019

massdosage Jan 24, 2019

fborriello Jan 24, 2019

fborriello left a comment

AnanaMJ Jan 24, 2019

fborriello Jan 24, 2019

AnanaMJ commented Jan 24, 2019

fborriello commented Jan 24, 2019

massdosage commented Jan 24, 2019

Modified configuration in order to work with java 11 #25

Modified configuration in order to work with java 11 #25

Conversation

fborriello commented Jan 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

massdosage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fborriello left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnanaMJ commented Jan 24, 2019

fborriello commented Jan 24, 2019

massdosage commented Jan 24, 2019