On 11 December 2011 22:42, Siegfried Goeschl <sgoes...@gmx.at> wrote:
> Hi folks,
>
> I ran the commons-email-1.2 test suite with commons-email-1.3 and got
>
> [junit] Running org.apache.commons.mail.EmailTest
> [junit] Testsuite: org.apache.commons.mail.EmailTest
> [junit] Tests run: 39, Failures: 0, Errors: 17, Time elapsed: 0.252 sec
> [junit] Tests run: 39, Failures: 0, Errors: 17, Time elapsed: 0.252 sec
>
> [junit] Testcase: testGetSetSession took 0.012 sec
> [junit] Caused an ERROR
> [junit]
> org.apache.commons.mail.mocks.MockEmailConcrete.setMailSession(Ljavax/mail/Session;)V
> [junit] java.lang.NoSuchMethodError:
> org.apache.commons.mail.mocks.MockEmailConcrete.setMailSession(Ljavax/mail/Session;)V
> [junit] at
> org.apache.commons.mail.EmailTest.testGetSetSession(EmailTest.java:108)
>
> Well, had another run with some other code getting
>
> ests run: 11, Failures: 0, Errors: 10, Skipped: 0, Time elapsed: 0.451 sec
> <<< FAILURE!
> testDefaultDomain(org.apache.fulcrum.commonsemail.CommonsEmailServiceTest)
> Time elapsed: 0.147 sec <<< ERROR!
> java.lang.NoSuchMethodError:
> org.apache.commons.mail.Email.setAuthentication(Ljava/lang/String;Ljava/lang/String;)V
Note the V at the end - that means void return.
> at
> org.apache.fulcrum.commonsemail.impl.CommonsEmailServiceImpl.configure(CommonsEmailServiceImpl.java:1007)
> at
> org.apache.fulcrum.commonsemail.impl.CommonsEmailServiceImpl.createSimpleEmail(CommonsEmailServiceImpl.java:285)
> at
> org.apache.fulcrum.commonsemail.CommonsEmailServiceTest.testDefaultDomain(CommonsEmailServiceTest.java:274)
>
>
> So Sebastian was right ...
>
> +) changing the return type from "void" to something else breaks binary
> compatibility
>
> +) moving the constants from Email to EmailConstants.java was had no impact
>
>
> Sebastian, kudos given ... :-)
>
Thanks; I only know this because I tested it when we had the IO issue
- we wanted to return non-void.
At first I did not believe it myself either: why should it matter if a
void method is changed to return non-void?
It cannot affect existing code, surely? However, that's not the way
the JVM works; the return type is part of the method sig. used when
finding the method.
[Of course it does not affect source compat; it will compile OK if the
return type is ignored].
> Cheers,
>
> Siegfried Goeschl
>
>
>
>
> On 11.12.11 22:06, Siegfried Goeschl wrote:
>>
>> Hi folks,
>>
>> reviewing the release candidate showed a few problems/discussion points
>>
>> 1) Moving constant from Email.java to EmailConstants,java
>> ==================================================
>>
>> I made the following change
>>
>> +) adding EmailConstants
>> +) Email implements EmailConstants
>>
>> public interface EmailConstants {}
>> public abstract class Email implements EmailConstants {}
>>
>> We have different opinions out there
>>
>> +) Gary - in general unhappy about an interface containing constants
>> only, and see issues with source code and binary compatibility
>
>> +) Sebastian - sees no issues with binary compatibility
>> +) I personally don't see any issues otherwise I would not have made the
>> change
>>
>>
>> 2) Renaming of protected fields
>> ==================================================
>>
>> The code exposes every field as protected which makes me unhappy since
>> the same filed could be accessed using a getter/setter as well. Due to
>> EMAIL-105 (https://issues.apache.org/jira/browse/EMAIL-105) I renamed
>> two protected fields on order to clarify the behavior
>>
>> * tls ==> startTlsEnabled
>> * ssl ==> sslOnConnect
>>
>> The original getters/setters are still there but deprecated now
>>
>> +) Sebastian pointed out that this breaks binary compatibility
>> +) I think along the lines that the protected fields should not be used
>> at all if there is a getter/setter available
>>
>> The question - does this change requires a new major release?
Potentially yes.
This is one of the reasons I keep saying that fields should be private.
The only reason for having a non-private field is a constant, i.e.
final, usually public static as well.
Mutable non-private fields make it much harder to show that code
behaves OK; they break data encapsulation rules.
Getters and setters protect the class against accidental or malicious change.
If there is a chance that the fields have been used by external code,
then renaming will break that code.
But before rushing to create a major release, consider whether it is
worth the effort of changing the package now.
Are there any other non-private mutable fields? Classes that should be
made immutable? Other API design errors?
I would make sure that all the non-private fields were deprecated, and
make sure that there are getters/setters instead.
If 3rd party code then misuses the mutable fields, and the code
misbehaves - well at least they were warned.
At a later point, do a thorough review of all the code, and make all
the API breaks at once.
Meanwhile use deprecation and Javadoc to document how to use the code safely.
>>
>>
>> 3) Return type of setters changed from "void" to "this"
>> ==================================================
>>
>> I changed the return type of setters to support something like this
>>
>> email.setMailSession()setDebug().setContent();
>>
>> instead of
>>
>> email.setMailSession();
>> email.setDebug();
>> email.setContent();
>>
>> +) Sebastian pointed out that this breaks binary compatibility (a
>> similar issues happened in commons-io)
>> +) based on my knowledge I doubt that but have to admit that Sebastian
>> knows a lot of things better than I do ... :-)
>>
>> I thing the way to go is to run the commons-email-1.2 test suite with
>> commons-email-1.3 and to report the result
>>
>> 4) The source zip is missing
>> ==================================================
>>
>> No discussions about that ... :-)
>>
>>
>> 5) OS-dependency in DataSourceFileResolverTest.testResolvingFileLenient
>> ==================================================
>>
>> No discussions about that ... :-)
>>
>>
>> 6) RAT Complaints
>> ==================================================
>>
>> The "mime.type" and four test email messages have no ASL - with the
>> newest commons-parent it should be possible to exclude files from RAT
>>
>>
>> I'm sort of stuck here - IMHO the changes regarding 1), 2) and 3) are
>> not big enough to justify a new major release whereas the library has
>> enough rough edges to look forward an clean-up and major release. But
>> for doing that I simple have not enough time for the next weeks ... any
>> opinions out there?
>>
>> Cheers,
>>
>> Siegfried Goeschl
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
>> For additional commands, e-mail: dev-h...@commons.apache.org
>>
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> For additional commands, e-mail: dev-h...@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
