[ckan-dev] test performance and sqlalchemy complete

[James Gardner]

I'm personally fine with this particular monkey patch and in this
specific case think it is less confusing than installing a forked
dependency.

James


On Thu, 2011-01-06 at 16:56 +0000, Seb Bacon wrote:
> On 6 January 2011 16:12, William Waites <ww at eris.okfn.org> wrote:
> > * [2011-01-06 14:28:03 +0000] Seb Bacon <seb.bacon at gmail.com> écrit:
> >
> > ] I've commited a monkeypatch now.
> >
> > please roll that back and just put a patched sqlalchemy up and refer to it in the pip requirements.
> >
> > third party apps monkey patching libraries wreak havoc with testing local changes, etc, to those libraries.
> >
> > please do not do this.
> 
> While I agree with the general point about monkeypatches:
> 
> (a) this patch, which only involves changing a flag from True to
> False, is only applied when running tests against a sqlite backend.  I
> can't think of a scenario whereby this would wreak any havoc?
> 
> (b) putting up a patched tarball of sqlalchemy isn't painless either.
> It creates a bit more cognitive-maintenance overhead for when we do
> sqlalchemy updates (e.g. how will whoever does an update in a year
> know why we have our own version? should we maintain the patch next to
> it somewhere for that? etc)
> 
> The point in (b) isn't a big deal, but I maintain neither is the
> potential issue with a monkeypatch, *in this specific case*.  Plus, I
> haven't got time to think about it again until next week ;-)
> 
> Have a look at the patch
> (https://bitbucket.org/sebbacon/ckan/changeset/ad9f51642166), and if
> you're still convinced it might screw things up I'll happily change it
> next week.
> 
> Thanks,
> 
> Seb
> 
> _______________________________________________
> ckan-dev mailing list
> ckan-dev at lists.okfn.org
> http://lists.okfn.org/mailman/listinfo/ckan-dev