You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Siegfried Goeschl <sg...@gmx.at> on 2011/12/11 22:06:33 UTC
[VOTE][CANCEL] The vote for commons-email-1.3 based on RC2 in cancelled
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?
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-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE][CANCEL] The vote for commons-email-1.3 based on RC2 in cancelled
Posted by sebb <se...@gmail.com>.
On 11 December 2011 22:42, Siegfried Goeschl <sg...@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-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [VOTE][CANCEL] The vote for commons-email-1.3 based on RC2 in
cancelled
Posted by Siegfried Goeschl <sg...@gmx.at>.
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
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 ... :-)
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?
>
>
> 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-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org