"Phillip J. Eby" wrote: > > At 08:58 PM 6/19/00 +0800, mike wrote: > > > >http://www.zope.org/Members/RainDog/ZSession/ZSession-0.0.2.tar.gz/view > > > >Comments? > > > > Now that I've had a chance to really look at this (while tracking down one > of the bugs you found), I do have a few comments. > > First, nice job... It's a good adaptation use of ZPatterns. I do have Thanks. > some suggestions for fine tuning, though. > > * Rather than putting property manipulation functions right into the base > Session class, I would simply let users derive ZClasses of their own from > Session. They could then actually give the Session a UI of its own, for I put those get/set/has stuff just for make easier life of some lazy people (like me :-) > example. Think of someone creating a Session subclass called "Shopping > Cart", with methods for viewing, checking out, adding/deleting items, etc. > Or, if they have many subsystems which want to share the same Session > objects, they could do this by having each subsystem use a different > propertysheet of the same Session object. Well, well, well. As for me, beter I've set up a Client abstraction which have access to CartManager and store cart_id in the session object. Mixing Session and Cart is not a good idea just because Client can have several carts, for example. The composition is better than inheritance in this case, Phillip :-) > IMHO, the basicSheet stuff you've done, while convenient, encourages > developers to take the quick-and-dirty route of just using the Session as a > place to store variables - and that throws away a lot of the usefulness of > ZPatterns. For one thing, you've guaranteed that Sessions have to be > stored as persistent objects in the ZODB, or at least that the basicSheet > has to support arbitrary properties. But without this stuff people will have to code ZClasses each time they want sessions. Hmm... I guess the best way is splitting current session class into two - simple Session and LazySession. > * Make dead session removal something that doesn't happen automatically, or > at least have an option to do so. Your default deletion algorithm would be > very expensive when executed against a non-ZODB database, and would be more > efficiently done a few times a day in large batches by a cron job. Moreover, I found a bug which causes infinite loop :-( You're right, automatic removal should be optional. Will be fixed in 0.0.3 release. > > * Delegate determining which sessions are dead to the Rack, rather than > doing it in the Specialist. This allows an SQL implementation to make use > of indexes. Specifically, make removeDead call: > > self._getSource().findIdleSessions(self.session_ttl) > > And then provide a default implementation of findIdleSessions() in the > Specialist. (Btw, please note that your current implementation absolutely > requires that at least *some* session data be stored persistently in the > ZODB, which if you're using a FileStorage, is a bad place for it to be.) > Now, the whole thing works the same way when first installed, but if > someone implements the session ID and lastAccessed attributes in an SQL > database, they can substitute a different implementation for > findIdleSessions() by putting it in the Rack. Agree with you completely. Thanks, Mike _______________________________________________ Zope-Dev maillist - [EMAIL PROTECTED] http://lists.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - http://lists.zope.org/mailman/listinfo/zope-announce http://lists.zope.org/mailman/listinfo/zope )
