Hi,
Ticket #422: Oh dear! I seem to have made a bit of a hash of this. Most of your comments relate to patch 1, which has actually already been committed. The other controversial bit was the change to ClauseList <http://www.sqlalchemy.org/trac/wiki/ClauseList>.compare. What I've done is roll up patches 2,3 and removed the controversial change. Please can you ignore patches 1,2,3 and consider committing patch 4. When I get a chance, I will submit a patch to fix the changes in patch 1 that you don't like. Regarding making ClauseList <http://www.sqlalchemy.org/trac/wiki/ClauseList>.compare order insensitive, this isn't important to me, I can recode the unit test to fix this another way. My logic is that this clause: a.a = b.a and a.b = b.b is logically identical to: a.b = b.b and a.a = b.a However, the compare function was showing them to be different. Perhaps changing ClauseList <http://www.sqlalchemy.org/trac/wiki/ClauseList>.compare like that is too sweeping. Instead, maybe we could change CompoundClause <http://www.sqlalchemy.org/trac/wiki/CompoundClause>.compare, so that if the operator is commutative (e.g. 'and', 'or') then it does an order insensitive compare. Tickets #462, #473 - can you close these down when you get a chance? The patch in #473 shouldn't be too controversial. Thanks for your help, and the great ORM! I showed this to someone at work today and he was really impressed. Paul Michael Bayer wrote: >yeah im not cool changing the compare method on ClauseList, thats a >very deep place to change something which should not matter for any >particular database dialect, youd have to show me what thats >accomplishing. the test is more strict as an "ordered" test and >theres been no issue with that as of yet. > >also, i dont like the "autoseq" table being added to the "tables.py" >module (nothing should be added to there which isnt globally used by >many tests/all databases) and also the "autoseq" test in the >"session.py" module, since its very ms-sql specific and the >"test_fetchid" functionality seems better suited for the test/sql/ >defaults.py test, which tests things like "autoincrement" columns and >such (without getting the ORM involved). > >the change from "opts" to "db_opts" in testbase.py should be done in a >style more consistent with what was already there... i dont see why >the name has to change. DBTYPE, if not specified, should be pulled >from a sqlalchemy.engine.url.URL object and not parsed..and also i >dont understand why the "auto_identity_insert" flag, if its so >critical to maintain compatibilty with SA's normal behavior, defaults >to False. at the very least have it parsed as a URL querystring >argument (see mysql.py for examples) so it can be part of the URL, >i.e. "mssql://<someurl>?auto_identity_insert=True", so that the global >**db_opts thing doesnt have to happen in the test suite. > >On Feb 11, 4:48 pm, Paul Johnston <[EMAIL PROTECTED]> wrote: > > >>Hi, >> >>Can someone with committer status take a look at the following when >>possible: >> >>#462 - Please close the ticket; I reopened for a reason that I now >>realise is duff (info on ticket). >>#422 - Gets more unit tests to pass for MSSQL. The patch does change the >>behaviour of _ClauseList.compare so the compare is order-insensitive. I >>think that is a good change semanticly, but if you disagree I can find a >>different fix. >>#473 - MSSQL bug fix. >> >>Many thanks! >> >>Paul >> >> > > >> > > > --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "sqlalchemy" group. To post to this group, send email to sqlalchemy@googlegroups.com To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/sqlalchemy?hl=en -~----------~----~----~----~------~----~------~--~---
