On Aug 19, Jay McCarthy wrote: > I hope I've incorporated much of the feedback from this thread: > > http://faculty.cs.byu.edu/~jay/tmp/201008161509-guidelines.html
I (finally) read this and the thread that went on at the time, and I don't see any point in all of this, besides a vage plea to encourage tests, and a slightly more concrete (but impractical) call for stress tests. * It talks about code in general -- no distinction between introducing new functionality, and fixing existing code. It looks like the emphasis is on the former, but that's not clear. These two cases are very different. * For the latter case of fixing someone else's code, much of this is irrelevant. If I see a bug in a library, and I tell the author about it but it is not fixed after a while -- if the fix is simple, I'll probably push it. (*I*'ll probably fix it right away.) But does that mean that the responsibility of keeping the code (and therefore maintaining a test suite) is now all mine? The text makes it look that way, but if these rules were enforced in some concrete way (for example -- you touch the slideshow code, and you get to be responsible for all its bugs) the net result would be no fixes. * So obviously some concrete ownership change is out of the question. Is there some implicit change, or something less radical like "you own your changes"? Probably -- but such things are hard to impossible to formalize in any way. I think that ultimately it's something that gets settled between the owner of the code and the person fixing it. As someone who was very often at the fixing side, I've just learned to deal with things on a case by case basis. In some cases the owner is happy for any fix, even if testing it is very difficult (or if the fix is obvious that there's no need for a test). In other cases the owner rejected any change and my fixes got dumped -- and I learned to completely avoid such pieces of code. It yet other cases the owner is relatively new, and I'll do more radical changes. The bottom line is that I don't see any formal way to formalize sane rules and keep moving at the same time. * "As long as your code fulfills its promises" seems like a weasel passage that can admit anything -- it's me who writes the code and me who does the promising, so I can always claim that I never promised [whatever you don't like about it, including bugs]. * There's an emphasis on stress testing -- but I don't see anything concrete there, and I don't see any way of making any concrete conclusions wrt such testing. To give a few examples: - We still have the `seqn' operators in, and they still imply a runtime cost of ~200. What should I do in light of the stress tests passage? - Add a test that will actually break, knowing that fixing it is not a matter of some local change to the code? - Remove the code, because a 200x is bad enough to be considered a bug? - Spend a month rewriting how sequences are represented to actually solve the problem? - Shut up about it, and learn to ignore it? - The unstable/queue code had a much smaller performance hit, but the code in question was simple enough to make that hit much more noticeable, so in that case even a small 5x factor is bad. - In Hari's code there is generally a similar performance hit, but in that case the added functionality (being pure) is worth it, and the code is less simple (certainly not something I'd want to write myself whenever I need purity). So a 5x factor is not a problem. * A minor note: the text refers to the tests/stress/stress library, but that code has no real documentation. (Looking at the code, there's some aspects that should really go into `time' -- like the ability to run the body multiple times, and average the results.) * "\"Primum non nocere\"" -- after looking this up (bad for such a document), I strongly disagree with it. IIUC, it reads as "if it works, don't mess with it" -- which is a recipe for an ever growing codebase with almost no changes to existing code. There are places where this does apply (eg, everything around drracket involve subtle state-based issues that random by-passers are unaware of), and places where it clearly should not apply. The question should really involve ownership: your own dialog with the owner of the code, or your willingness to become its owner. * Speaking about ownership -- IMO it is one of the important aspect of any "coding guidelines", yet there is nothing about it in the text. So IMO this is a huge omission. More specifically, much of my criticism of the unstable collection is related to ownership -- code being dumped as is and left as an orphan with noone to take care of problems. (Yes, there's a policy of clarifying the owner of each bit, but the bottom line is that except for a few things, there was no discussion on it and many of the comments that I wrote are unanswered.) * Thinking about this, I think that a proper document would be something that makes me view the unstable collection more favorably. (No, that's not impossible.) Right now, it looks pretty decent when judging by these guidelines -- the functions are all documented and tested -- the problems are in other aspects. * Yet another huge ommision from such a document is style. This comes in several flavors: - Low-level aspects of the files: no newline at the end of a file, using tabs, empty lines in weird places, spaces at end of lines, etc. - Higher level: inconsistent indentation, use of "#lang" vs "(module ...)", style of in-code comments, etc. - Even higher level: consistent naming schemes for module exports, deciding what is part of the external API and what's internal, splitting a library into too many or two few modules, splitting a module into too many or too few functions, adding too many expensive contracts or not adding contracts at all, dealing with code dependencies. - Also includes meta-information: what should commit messages include? what group of changes makes a good commit? what should be included in release announcements? why don't we have a changelog? -- ((lambda (x) (x x)) (lambda (x) (x x))) Eli Barzilay: http://barzilay.org/ Maze is Life! _________________________________________________ For list-related administrative tasks: http://lists.racket-lang.org/listinfo/dev