[Building Sakai] MT deprecation recommendationsfor 2.7

Sun Mar 21 06:32:18 PDT 2010

I like reading Sakai threads that include a reply that injects passion as well as polemic into the debate.  It demonstrates (again) that people care about this project and the code that undergirds it.  Anyways, here's my take on a few of the points you raise:

> I think the fallacy in all of this discussion is that somehow there is a strong feeling in the community that "Static Covers Must Be Removed" - just having a Confluence page that says "Best practice is removing static covers" does not a mandate make.  Does this mean that if I log into the confluence page and edit the text to say "Static covers are best practice" - that the community intent changes?  Perhaps I should do that and then cite my page as "a mandate".

Nah, no one participating in this discussion is that naive.  Indeed, anyone familiar with the history of Sakai technical development knows that the issue of static covers is a contested one.  David Horwitz chose to cite the kernel "best practices" page--in which the "no usage of static covers" injunction was added in v. 3 on 21 Sep 2007--simply to highlight the antiquity of the concern.  In my case I cited a 2007 blog post of Ian Boston's (http://blog.tfd.co.uk/2007/09/05/unit-testing-strategies/)--again, to highlight the fact that the MT recommendation is by no means a mere whimsey but has some roots.

The pity here is that the kernel "best practices" page in Confluence and it's apparently bold assertion regarding static covers (i.e., "don't use them") has sat unedited and uncommented upon since Dec 2008.  Perhaps it was a bit presumptuous of the original author to title the page "Best Practices for Kernel Code" -- but then again the idea that someone would step up and attempt to elucidate a set of coding best practices with respect to the kernel is, I think, laudable.  What the page could use is an injection of crowd wisdom -- that it hasn't occurred is not the fault of the original author.

So yeah, log into Confluence and have a go at the page or at least add a cautionary comment.  It would be good for new developers to know that agreement on this "best practice" is by no means universal.

> Your proposed "compromise" is not a compromise at all - it lets the anti-cover faction "win" over the "anti-anti-cover" faction.

Presume innocence.  There are no factions here, only friends.   And if there is a win to be had it's probably over in QA.

> Probably the thing that bothers me off the most is when the MT comes through my code and removes an elegant single line import of a cover and replaces it with 25 lines of setter, getter, and comments and adds 10 lines of XML in some obscure file - and after they are done and have left with a smug sense of accomplishment - I have to maintain that brittle and ugly code.

A great sound bite but hardly reflective of actual practice or a fair characterization of code that uses setter injection.  The MT's writ does not extend beyond the (abandoned) code that its members have agreed to take over and support.  The team respects project boundaries and won't cross them without permission (as was made clear in the original proposal and elsewhere).  So the code you contribute is safe.

I do find it hard to accept the claim that replacing a cover with setter injection leaves one with inelegant, brittle and ugly code.  Plus, it's incorrect to say that dependency injection requires the addition of a get method.  It doesn't.  But code aesthetics is a deeply personal matter so I hesitate to dismiss the assertion as a rhetorical flourish rooted in hyperbole.

We should focus on concrete illustrations of the issue.  I earlier cited SAK-12077 (http://jira.sakaiproject.org/browse/SAK-12077) as an example of what it takes to replace static covers with setter injection.  Nothing onerous or difficult in the exercise but it does take time and is non-trivial.  Stephen Marquard has pointed out correctly that wholesale replacement of static covers could prove burdensome for schools maintaining local/contrib code.  I agree and suggested that we deal with the issue of static cover replacement incrementally but decisively in cases where static covers impede the writing of unit tests.  I believe deprecating all covers now is the right move since it encourages developers to write unit-test friendly 2.x code, but that is an opinion.

> But like with all these "patriotic" proposals, conservative "maintain the status quo" arguments to minimize pain and wasted effort always lose against "patriotic pro-glorious-future" arguments and I will just grumble and be forced to follow along. :) 

Hah, a bit over the top but I like the barely concealed allusion to contemporary debates in the US on health care reform.  It suggests linkages to a meta-historical narrative or discourse to which most of us in our role as developers are blithely unaware.  But I don't buy it.  There's nothing patriotic, glorious or inevitable in the covers debate.  It's all rather grubby actually.  Besides, as I noted earlier, the kernel team is split over static cover deprecation/removal and since all 2.x roads lead to the kernel, no wholesale "clean up" is likely ever to take place.  

So no need to grumble.

Cheers,

Anth

On Mar 20, 2010, at 7:06 PM, csev wrote:

> Anthony,
> 
> I think the fallacy in all of this discussion is that somehow there is a strong feeling in the community that "Static Covers Must Be Removed" - just having a Confluence page that says "Best practice is removing static covers" does not a mandate make.  Does this mean that if I log into the confluence page and edit the text to say "Static covers are best practice" - that the community intent changes?  Perhaps I should do that and then cite my page as "a mandate".
> 
> The very fact that the discussion goes round and round suggests that the community is not in agreement and the different people have different trade-offs.
> 
> Your proposed "compromise" is not a compromise at all - it lets the anti-cover faction "win" over the "anti-anti-cover" faction.
> 
> I think that the real compromise is to do everything that MT folks want to do *except* marking them as deprecated and then making the deprecation decision later after the MT has a lot of experience in "static cover cleanup" - perhaps they will tire of the game after they see how much cleaner static covers are than Spring + Setters.
> 
> All that marking them as deprecated immediately does makes the long-suffering anti-cover folks feel they have a "moral victory" that does not really improve anything at the cost of increasing pain for everyone else who is stuck either investing effort in changing their code or learning to ignore lots of deprecation warnings.
> 
> Probably the thing that bothers me off the most is when the MT comes through my code and removes an elegant single line import of a cover and replaces it with 25 lines of setter, getter, and comments and adds 10 lines of XML in some obscure file - and after they are done and have left with a smug sense of accomplishment - I have to maintain that brittle and ugly code.
> 
> The other thing that such a useless re-factor does is makes it increasingly difficult for folks on earlier versions of Sakai to pull current Sakai code back into those earlier branches.
> 
> But like with all these "patriotic" proposals, conservative "maintain the status quo" arguments to minimize pain and wasted effort always lose against "patriotic pro-glorious-future" arguments and I will just grumble and be forced to follow along. :) 
> 
> /Chuck
> 
> On Mar 20, 2010, at 10:02 AM, Anthony Whyte wrote:
> 
>> The all-or-nothing cast to this discussion is going to lead us nowhere fast.  I put forward a compromise proposal on 17 March and echoed by David Haines on the 19th that offers us incremental, case-by-case approach.  It recognizes that there may be times when a static cover needs to be sacrificed in the interests of providing unit tests that help strengthen the bug fix validation and verification process.
>> 
>> Ensuring that core production code is stable, reliable, maintainable and defect free (to the best of our abilities) trumps, in my view, (inexperienced) developer conveniences or local/contrib implementation decisions.  That said, as Stephen Marquard has pointed out, deprecating and then removing static covers wholesale could well impose extra costs on schools required to refactor static covers out of their local/contrib code.  This needs to be recognized but should not impose an absolute blocker on static cover removal.
>> 
>> So, 
>> 
>> 1.  Deprecate static covers in trunk/2.7.x (this at least provides a Javadoc warning that a cover might in future be removed)
>> 2.  Add further documentation in Confluence regarding Sakai developer best practices with respect to covers.
>> 3.  Address any static cover removal question on a case-by-case basis in Jira and on list (perhaps limited to blocker/critical issues).  
>> 4.  If a static cover removal is justified, do it.
>> 
>> Cheers,
>> 
>> Anth
>> 
>> 
>>> COMPROMISE PROPOSAL (17 March)
>>> I'd like to see static covers eventually removed (so +1) but recognize that the current crowd wisdom appears to consider this desire, well, unwise.  Still, I'd like to suggest a compromise proposal, one that permits the removal of a static cover whenever it impedes the implementation of unit tests written in support of a bug fix.  Extension of 2.x unit test coverage is work that meets a long standing yet largely unrealized goal of the Sakai Community.  Suspending such work because one finds a static cover in the way appears to me unwise.
> _______________________________________________
> sakai-dev mailing list
> sakai-dev at collab.sakaiproject.org
> http://collab.sakaiproject.org/mailman/listinfo/sakai-dev
> 
> TO UNSUBSCRIBE: send email to sakai-dev-unsubscribe at collab.sakaiproject.org with a subject of "unsubscribe"
> 
> 
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 2417 bytes
Desc: not available
Url : http://collab.sakaiproject.org/pipermail/sakai-dev/attachments/20100321/1d2d4977/attachment.bin 