On 12 April 2011 18:59, Daniel F. Savarese <d...@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-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
