On Jun 4, 2012, at 10:16 AM, Ask Bjørn Hansen wrote: > On Jun 4, 2012, at 9:32, Charlie Brady wrote: > >>> -use Time::HiRes qw(gettimeofday tv_interval); >>> use Qpsmtpd::Constants; >>> >>> +use Time::HiRes qw(gettimeofday tv_interval); >> >> Is the change in ordering here accidental, gratuitous, or is there some >> hidden functional significance? > > I think Matt has been trying to make loading other Qpsmtpd modules go first > and then "external" dependencies second. Almost all the time that's fine (no > functional change) and it is neater.
Precisely. > (It's however also what makes reviewing these "cleanup patches" so much work, > each change has to be considered -- was this just reformatting? Was it > intentional? Almost all of my non-functional changes are ones you'll find listed in PBP, and they resolve around making code easier to read and maintain. Quite often I get into a bit of code and find it's no small task to figure out what's happening. Especially long chains of conditionals. They carry a high cognitive load. Most of the changes I make are covered by these few principles: Code in commented paragraphs. Explicit returns. Linear coding (Reject as many iterations as possible, as early as possible.) Avoid negative control statements I try to make these changes first, without changing any functionality. Then I write the tests. Finally, I go in and start changing how things work, which is mostly extending the functionality so that it also works the way I want. But in such a way that it's easy for someone else to change it to work the way they want. For example, the changes in github have scores of changes that revolve around how a plugin rejects a message. Scores of them. Because there's no standard way to change how a plugin rejects messages without altering the plugin. With a small additional to Plugins.pm (https://github.com/smtpd/qpsmtpd/pull/23/files), all those changes go away, any qpsmtpd user (or developer) change how all their plugins handle mail, and every plugin supports these options: plugin reject [ 0 | 1 | naughty ] plugin reject_type [ perm | temp | disconnect | temp_disconnect ] That covers 99% of the changes that have been made to plugins to alter how they reject mail. Answers to FAQ's like "how do I use spam to train my spam plugins" can be written. Stuff that requires a developer now can be done by a user. > Does it change functionality? > What are the side-effects? That's what test coverage answers. :-) > -- in other words, help reviewing would be most welcome! :-) ) Another solution is just to review changes less. If it comes in with tests, and it doesn't stomp all over the guidelines laid out in docs/*.pod, then just let them pass. I realize there's only one branch of qp, but git is a VCS. If a commit breaks things, it's easy enough to back out. Ideas that don't work out are easy enough to prune later. Making room for other people to change and use your software in ways you didn't expect is, after all, the UNIX way. Matt
