jaykataria1111 commented on issue #2769: URL: https://github.com/apache/logging-log4j2/issues/2769#issuecomment-2461336252
> Note: I noticed that some of our builders have complex conventions for the name of the setter: the setter for an isUnicode field should be called setUnicode and not setIsUnicode. +1. I removed the type check and kept the naming check and fix the compilation issues for the purpose. I had to change the name of some fields due to this. It should be a backwards compatible change. >Instead of checking the type of the parameter, it would be more useful to check the return type of the setter: the return types of our setters are not always equal to the type of the builder (e.g. they can be type variables for abstract builders), but the return type should be assignable to the builder type. I believe that [Types.isAssignable()](https://docs.oracle.com/javase/8/docs/api/javax/lang/model/util/Types.html#isAssignable-javax.lang.model.type.TypeMirror-javax.lang.model.type.TypeMirror-) should handle that, although I am not sure how well it manages parameterized types I did incorporate that in code: Some of the methods had a void return type I fixed that for example changed [SocketPerformancePreferences](https://github.com/apache/logging-log4j2/blob/d229eda077c313a8a2060caa42531f6b539b1a09/log4j-core/src/main/java/org/apache/logging/log4j/core/net/SocketPerformancePreferences.java#L78-L88) ``` public SocketPerformancePreferences setBandwidth(final int bandwidth) { this.bandwidth = bandwidth; return this; } public SocketPerformancePreferences setConnectionTime(final int connectionTime) { this.connectionTime = connectionTime; return this; } public SocketPerformancePreferences setLatency(final int latency) { this.latency = latency; return this; } ``` The issue is that, the check-api-comp build task fails for the purpose when I do the code change, should we ignore it? All of my changes so far are backwards compatible, but the tool thinks that these are "MAJOR" changes. I have created a dummy PR in my own repo: [Code Changes](https://github.com/jaykataria1111/logging-log4j2/pull/1/commits/44695395e86b1439e15fbe778e2d58a0dc5037c6) to show you the code changes and how they should be backward compatible, please let me know if that works, also I am new to github, so learning its quirks, My company uses its own system, so workflows code reviewing are a bit different. So please bear with me if it is not a fancy PR and just my repo, as long as you can see the commit and the diff that is all that matters to me :) >Besides, the main utility of having setters is to be able to change the type of the field, without breaking backward compatibility. 😃 , yeah it is interesting to see, this sort of usage of setters, I must say I wasn't acquainted it with it, which just gives me more breadth of knowledge. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
