Hi Hanno > Betreff: Re: [Zope-dev] [Checkins] SVN: zope.publisher/trunk/ > Added some BBB code to setDefaultSkin > > Hi. > > Roger Ineichen wrote: > >> Roger Ineichen wrote: > >> Then there's some tests and description missing proving > that intent. > >> All I could read in the changelog and the skinnable tests was > >> pointing in the other direction: Making it possible to use the > >> Skinnable concept without relying on IBrowserRequest. The whole > >> JSONRequest (which is not a > >> BrowserRequest) tests inside skinnable.txt continue to work. > > > > Yes, I think everything was tested, but probably future > ideas are not > > documented. There where some discussion about to split each request > > into it's own packages. > > Where did that discussion happen? All I have heard of was > discussions at PyCon, where nobody quite seemed to see any > point in the whole different request types story at all anymore. > > I don't mind if the skinnable story gets less intermingled > with the request story in a new zope.skinnable package and > breaks some backwards compatibility at that point. Right now > that mix of the two concepts is so prevalent in all kinds of > places, that I'd rather stay on the backwards compatible side.
Can you tell me what was not backward compatible? > All ZCML browser directives by default register everything > for IDefaultBrowserLayer and expect those resources to be > available on "normal" browser requests. The test helpers in > zope.app.testing to get browser resources set up rely on the > same mix of concerns. This stuff is used all over the place > without caring about loading zope.publisher's ZCML right now. Did my refactoring break anything? If so what? > >> All I did here was to move two constructs from ZCML into > direct code. > >> The lines I added do exactly the same as the default adapter > >> registered as: > > > > Uhh, this is call hard coded and makes it impossible to exclude the > > adapter with the <exclude> directive. > > I call that retaining sensible defaults. You can opt-out of > the IDefaultBrowserLayer for browser requests by providing > your own specialized adapter. I prefer no configuration for > the most common case with an opt-out scenario instead of the > everything is opt-in behavior. Do you understand the impact of your changes? I see what you are trying to do but I don't know which package or application you are trying to fix with your changes. Can you give me some hints about what you are trying to fix with your changes? > > I see two things which are bad. Skinnable depends now on > > IBrowserRequest. I moved the skinnable concept out of the browser > > request part. This allows us to separate the skinnable and all > > different request types into own packages if we do future > refactoring. > > zope.publisher is the package that defines the > IBrowserRequest interface. It might make sense to split those > concerns off into different packages and then straighten out > the dependencies. At that point I can see having a > setDefaultSkin method inside zope.skinnable with a different > behavior. But the one inside zope.publisher ought to play > nicely with IBrowserRequest. Any improvements are very welcome. Do you think we should move the skinnable concept into zope.skinnable? Sorry if I'm bother you about this details but I spent a full day with this refactoring and one of my apps depends on this deault browser layer less concept. You just reverted the hole refactoring with 3 lines of code. Regards Roger Ineichen > Hanno > > _______________________________________________ > Zope-Dev maillist - Zope-Dev@zope.org > http://mail.zope.org/mailman/listinfo/zope-dev > ** No cross posts or HTML encoding! ** (Related lists - > http://mail.zope.org/mailman/listinfo/zope-announce > http://mail.zope.org/mailman/listinfo/zope ) > _______________________________________________ Zope-Dev maillist - Zope-Dev@zope.org http://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - http://mail.zope.org/mailman/listinfo/zope-announce http://mail.zope.org/mailman/listinfo/zope )