commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: Commons Net issues
Date Tue, 12 Apr 2011 18:46:38 GMT
On 12 April 2011 18:59, Daniel F. Savarese <dfs@savarese.org> wrote:
>
> 1. NET-396 POP3.setState() should not be public
>
>   I don't think I can revert a change without first getting permission
>   from the committer who made it.  I believe this change needs to be
>   reverted for the reasons stated in my issue comment.  sebb, if you
>   object, let's discuss it.  If you don't, please let me know so I can
>   revert the change.  I understand why you made the change.  However,
>   even though base protocol classes are intended primarily for use by
>   the libary, they are not exclusively for use by the library.  I think
>   changes of this sort that may make existing user code non-functional
>   should be left for a redesign and rewrite of Commons Net.  They don't
>   fix anything and may only break things.  In the case of this change,
>   I know it will break things for one of my former customers.

OK, fine, let's revert that change.

> 2. NET-393 Should the sendCommandWithID() methods be public?
>
>   I'm not asking to revert this change because I'm not familiar enough
>   with the IMAP code.  However, the person who contributed the IMAP code made
>   the IMAP base protocol class consistent with the pattern followed by the
>   existing base protocol classes, whereby all protocol manipulation functions
>   are supposed to be public.  As with the POP3 case, methods that have been
>   made public to allow developers with special needs to implement their own
>   protocol client code via aggregation should not be made non-public to
>   satisfy a sense of aesthetics or to protect programmers from doing stupid
>   things.  Unlike the POP3 case, I don't know of anyone for whom this change
>   will cause a problem.

The IMAP code is brand new so there are no existing users.

As I wrote on the JIRA, reducing visibility breaks binary
compatibility, so it is safer to start with minimum visibility and
relax later if it really proves necessary.

> 3. NET-402 BufferedReader used for control channel, which does not follow
>           the standard
>
>   The premise of this issue, at least with respect to NNTP and SMTP, is
>   misleading.  Both NNTP and SMTP prohibit the occurrence of a bare CR
>   or LF.  Therefore, the only standards-compliant behavior is to reject any
>   occurrences of bare CR or LF characters (which is undesirable if software
>   is to function in the presence of errors).  Recognizing only CRLF
>   as a valid line terminator does not make the code standards compliant
>   because now bare CR's and LF's are being interpreted as either
>   command contents or data, both of which are prohibited.

That may well be the case, but I could not find that prohibition.

>   Either you assume the server is standards compliant or it is not.  If
>   you assume it is standards compliant, then using BufferedReader is not
>   a problem.  If you assume it is not standards compliant, then
>   BufferedReader and the added CRLFLineReader both have problems in different
>   situations.

Only if bare CR or LF are never allowed, otherwise using
BufferedReader is a problem, and CRLFLineReader is an improvement.

>   Yada, yada, yada.  All of this has been considered before
>   and the decision was made to use BufferedReader because various
>   servers for a number of protocols at the time were known to be sending
>   bare LF's as line terminators instead of CRLF's.

That was presumably before I got involved, because I'm not aware of
that decision, and as far as I can tell it's not documented.

>    If it is more prevalent
>   for non-conforming servers to include bare CR or LF characters as
>   data instead of the end of a command, then it's better to accept them
>   as data.  But that wasn't the argument made for making the change and
>   would seem to apply to NNTP and POP3, but not necessarily SMTP.  A
>   complete solution (that I don't feel is worth implementing) would be to
>   identify what type of non-conformance one is faced with when encountering
>   a bare CR or LF (are we reading a command or data, does it look like we're
>   at the end of a command, etc. and decide what to do with the bare CR or LF).
>
>   It's a waste of time to revert this change as it only replaces one
>   problem with another.  However, if we are to build any sense of community
>   amongst people who have contributed to Commons Net development and
>   encourage more participation, this is exactly the sort of issue that
>   should be discussed on the dev list first before taking action.
>   Ultimately, the same decision and result may be reached, but at least it
>   will have been decided via a consensus-based approach.

Since JIRA issues are reported to the mailing list, I assumed that
developers would comment if they disagreed. I took the view (possibly
incorrectly) that these weren't major changes which required a
separate discussion.

> I hardly comment about Commons Net changes because I don't feel
> I contribute enough anymore to have a say.  But #1 is important because
> the POP3 class can be and has been used outside of the libarary.  I tend
> to get concerned only about changes that may adversely impact existing
> users and try to refrain from kibitzing needlessly when other people are
> doing all the hard work.  But Commons Net has become more and more
> fragile as it's been touched by so many different hands under different
> design assumptions.  In the interest of not breaking functionality for
> the users, I recommend we not make changes like 1 and 2 unless there's
> a known behavioral bug (which should be impossible for that class of change).
> I didn't really want to broach 3 because the issue itself impacts only
> exchanges with non-conforming servers and I have no desire to make
> mountains out of molehills.  But I felt I should say something because
> not discussing issues like this before making a change has whittled away
> the committership, leaving us with one active committer (sebb) and one
> largely inactive backup committer (me).  This isn't a phenomenon specific
> to Commons Net and it's not any easy one to solve because everyone has
> different notions of what sort of changes require discussion.

I agree that making changes to mainline code behaviour should not be
done without a JIRA, and in some cases a dev discussion is also
needed, but I don't think it's necessary to discuss every change on
the dev list.

> daniel
>
>
> ---------------------------------------------------------------------
> 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


Mime
View raw message