Use new version of site and thoughts about new release#1
Use new version of site and thoughts about new release#1KroArtem wants to merge 1 commit intoapache:masterfrom
Conversation
Cleanup code to align with java8 features Mark VerificationResultPrinter as a @FunctionalInterface Use java8 Update dependencies
michael-o
left a comment
michael-o
left a comment
There was a problem hiding this comment.
There are too many different changes here:
- Plugin updates must be separate
- Java update must be advertised separately, at least a minor version bump is required
- Java 7 style code separately
Please start with these, we will go on with the rest afterwards.
|
Yes, I know the changes are altogether in one PR but it's a sample PR. I've already sent a message on dev@maven.apache.org regarding this plugin. To summarize:
|
|
Hard to tell, the dependent ones haven't been updates for years -- nor they cared for this plugin to be updated. I'd rather discontinue this plugin. We have |
|
Let's wait for developer community replies then. |
elharo
left a comment
elharo
left a comment
There was a problem hiding this comment.
Some of this is good, especially updating dependencies, but personally I find adding lambdas to be a step backwards for readability. Others may feel differently. In general it's easier to move changes forward when they're split into smaller, more focused PRs, each of which does exactly one kind of thing.
|
|
||
| Reader reader = null; | ||
| try | ||
| try ( Reader reader = new FileReader( this.verificationFile ) ) |
There was a problem hiding this comment.
not changed in this PR, but this is likely a bug. There's no specified character set
There was a problem hiding this comment.
Indeed. There are no open issues about it, though, and according to this answer there is no possibility to set encoding for FileReader until Java 11. https://stackoverflow.com/questions/696626/java-filereader-encoding-issue
(another way still exists, though)
There was a problem hiding this comment.
Yes, this is why code shouldn't use FileReader. It's a bad API.
There was a problem hiding this comment.
good catch - @KroArtem could you create Jira and pr to fix that?
maybe this can be useful https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#newBufferedReader(java.nio.file.Path,%20java.nio.charset.Charset) ?
| <dependency> | ||
| <groupId>org.apache.maven.plugin-tools</groupId> | ||
| <artifactId>maven-plugin-annotations</artifactId> | ||
| <version>3.6.0</version> |
There was a problem hiding this comment.
If there wasn't a version here before you probably don't want it now. I'd guess it's supplied by thedependencyManagement section of a parent or some such thing.
There was a problem hiding this comment.
Well, I didn't find out any information about this.
|
thx for contribution. I've created https://issues.apache.org/jira/browse/MVERIFIER-36 for that and selected only changes related to move to Java 7 and code cleanup. I've also checked where we use that plugin ... and there is no big demand for it. We use it only in the integration suite and that's good reason to release version 3.0.0 and drop Maven 2 compatibility. Even if we then retie plugin. |
|
The build was successful - code merged to master. Thx. Let's continue the discussion about the release on the mailing list. |
|
@slachiewicz , thanks! Is there any reason to leave Java 7 version? |
|
No real benefits thanks to the Java 8 upgrade for this small plugin. And if we update only this plugin, it will be useful only for the basic Maven project (where we do not use it) and until we upgrade other plugins to a higher version of Java - we can not use it. |
4 participants
[description is a wip]
Cleanup code to align with java8 features
Mark VerificationResultPrinter as a @FunctionalInterface
Use java8
Update dependencies