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. 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.

...

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.

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. Hanno

Roger Ineichen wrote:

...

Betreff: Re: [Zope-dev] [Checkins] SVN: zope.publisher/trunk/ Added some BBB code to setDefaultSkin

...

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?

The setup of the default skin layer now required to load some ZCML from the zope.publisher package. So far Zope2 didn't load any of that ZCML and lots of test cases all over Zope2 (for example OFS, ZPublisher and parts of Five) don't use any kind of ZCML aware layers. Crafting a request via a simple dictionary and using it in rather straight unit tests is extremely common in Zope2 and things like CMF and Plone. People do combine that with the helper functions in zope.app.testing to get browser components of some kind set up easily. This is a typical "grown" story that isn't particular well defined and shouldn't block well devised future refactorings of the skinnable concept. I'd just rather not break tons of tests now, with half a refactoring in zope.publisher. If we get a real refactoring of the skinnable concept into its own package and can simplify the CA-overuse in it (things like interface classes used as adapters which implement interfaces themselves make my head hurt), I think it time to break backwards compatibility. I'd rather see methods like setDefaultSkin, which doesn't happen to set a default skin anymore be renamed to configureSkin or something else for example and let the setDefaultSkin method still do what it names suggests.

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.

I didn't revert the major part of the refactoring, which was about disentangling the two concepts and making it possible to use skins for other types of requests. I still don't understand why you don't override the default adapter that uses getDefaultSkin as a factory with one that returns a IDontWantTheDefault skin. You have to use an override instead of the exclude pattern, but given that nobody else seems to have had that particular use-case of yours so far, I tend to believe it's an uncommon edge-case scenario and the override pattern makes perfect sense to use for this. Hanno