Re: [Zope-dev] SVN: zope.app.component/trunk/ Please, don't just remove things that could be used in users code.
Dan Korostelev wrote:
Log message for revision 96156: Please, don't just remove things that could be used in users code.
Changed: U zope.app.component/trunk/CHANGES.txt U zope.app.component/trunk/src/zope/app/component/metadirectives.py
-=- Modified: zope.app.component/trunk/CHANGES.txt =================================================================== --- zope.app.component/trunk/CHANGES.txt 2009-02-05 18:07:51 UTC (rev 96155) +++ zope.app.component/trunk/CHANGES.txt 2009-02-05 18:22:19 UTC (rev 96156) @@ -5,7 +5,9 @@ 3.6.1 (unreleased) ------------------
-- ... +- Make ``class`` directive schemas importable from old location, + raising a deprecation warning. It was moved in the previous release, + but some custom directives could possibly use its schemas.
Out of curiosity, *was* it used by custom directives that you know of? A more philosophical question: are the interfaces used to implement a directive part of a public API that is reusable or not? When we moved the directive implementation I arbitrary made the assumption that it'd be fine to just move them wholesale, but you clearly don't agree. Besides the interfaces, should the implementation also have backwards compatibility imports? I guess sometimes reuse by interface subclassing does happen in directive implementation, but of course we could declare that such reuse is only intra-package. In my mind, it's also less risky to move a directive implementation even if it is reused, as the amount of external reuse is likely rare and is less likely to be in end-user code. The developers of the code layered on top of the directive definition would therefore be quite capable of fixing any breakage quickly. But of course placing a few imports for backwards compatibility is not a lot of work and may be the easiest way out of this set of questions. :) Regards, Martijn
+- Make ``class`` directive schemas importable from old location, + raising a deprecation warning. It was moved in the previous release, + but some custom directives could possibly use its schemas.
Out of curiosity, *was* it used by custom directives that you know of?
Well, I personally found out that they were removed while checking out the z3ext packages. Some of them use "require" and "allow" subdirectives for their custom directives. In the case of "allow" and "require" it's quite easy to think out the case where they will be reused.
A more philosophical question: are the interfaces used to implement a directive part of a public API that is reusable or not? When we moved the directive implementation I arbitrary made the assumption that it'd be fine to just move them wholesale, but you clearly don't agree. Besides the interfaces, should the implementation also have backwards compatibility imports? I guess sometimes reuse by interface subclassing does happen in directive implementation, but of course we could declare that such reuse is only intra-package.
It looks like "yes, it should" to me. AFAIK, we reuse directives in zope itself and in other packages. One example is z3c.pagelet that reuses IBasicViewInformation schema and functions like "handle_permission" or "handle_for". However, that might be considered as a flaw to be fixed, so there's a place for discussion on that topic. However, I think that it's even good for directives to be a part of reusable API. What if we want to create an extended <browser:page /> directive, adding only one custom field? The implementation functions and classes are also reused sometimes. We reuse the zope.component's directives like "adapter" and "utility", so why not reuse a ClassDirective if it fits our needs?
In my mind, it's also less risky to move a directive implementation even if it is reused, as the amount of external reuse is likely rare and is less likely to be in end-user code. The developers of the code layered on top of the directive definition would therefore be quite capable of fixing any breakage quickly.
Of course, its clearly okay to move directive schemas and definitions around, but I believe, that we need to support old imports for one or two "big releases" as we did before. There's a nice deprecation framework, provided by zope.deprecation and zope.deferredimport, so why not just use them to maintain backward compatibility while signaling people that they need to update their code? Not everyone is so quick and has time to fix the breakage. It's also quite hard to qucikly find where did the thing move if there's no compatibility imports or comments w/o reading changelogs.
But of course placing a few imports for backwards compatibility is not a lot of work and may be the easiest way out of this set of questions. :)
Yep. Also, as I said before I think we also need to use deprecation warnings for imports that are not classes for persistent objects (until Chiristian writes the tool to upgrade them :)). -- WBR, Dan Korostelev
Hey, On Fri, Feb 6, 2009 at 11:32 AM, Dan Korostelev <nadako@gmail.com> wrote: [snip] Okay, reuse of directive definitions and implementations is more widespread than I thought initially. Which is silly of me, as Grok itself also reuses directive actions (but not definitions) in some places.
But of course placing a few imports for backwards compatibility is not a lot of work and may be the easiest way out of this set of questions. :)
Yep. Also, as I said before I think we also need to use deprecation warnings for imports that are not classes for persistent objects (until Chiristian writes the tool to upgrade them :)).
As far as I understand in a recent discussion people indicated they didn't like deprecation warnings anywhere, as they are forced on third party users of code that itself isn't deprecating anything. Since Christian's tool is as I take it well underway couldn't we just rely on this? Also since it actually *fixes* the state of a ZODB, instead of just giving a lot of warnings and then leaving the helpless developer with writing some scary custom upgrade code. Regards, Martijn
2009/2/6 Martijn Faassen <faassen@startifact.com>:
Yep. Also, as I said before I think we also need to use deprecation warnings for imports that are not classes for persistent objects (until Chiristian writes the tool to upgrade them :)).
As far as I understand in a recent discussion people indicated they didn't like deprecation warnings anywhere, as they are forced on third party users of code that itself isn't deprecating anything. Since Christian's tool is as I take it well underway couldn't we just rely on this? Also since it actually *fixes* the state of a ZODB, instead of just giving a lot of warnings and then leaving the helpless developer with writing some scary custom upgrade code.
I don't think there was a consensus about that. For ZODB objects I think its okay for now not to use deprecations and to use the upgrade tool, but for the imports of other things that were moved elsewhere, I still think we need to bug developers that they need to upgrade their code with deprecation warnings, so we can eventually remove old imports. Oh, and BTW, if we use the "zodb fix tool", it's also okay to raise deprecation warnings about ZODB objects, as it's really easy to fix with the magic tool and again, we can eventually remove the deprecated import and make our code more clean. Let's discuss it once again :) -- WBR, Dan Korostelev
Hi there, On Fri, Feb 6, 2009 at 12:10 PM, Dan Korostelev <nadako@gmail.com> wrote: [snip]
For ZODB objects I think its okay for now not to use deprecations and to use the upgrade tool, but for the imports of other things that were moved elsewhere, I still think we need to bug developers that they need to upgrade their code with deprecation warnings, so we can eventually remove old imports.
The new test runner can give such warnings for many indirect imports (for classes and functions, but not for instances). I also think that we should highlight the importance of documentation and communication for deprecation. Any tool is not a substitute for communication about changes. Regards, Martijn
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Dan Korostelev wrote:
2009/2/6 Martijn Faassen <faassen@startifact.com>:
Yep. Also, as I said before I think we also need to use deprecation warnings for imports that are not classes for persistent objects (until Chiristian writes the tool to upgrade them :)). As far as I understand in a recent discussion people indicated they didn't like deprecation warnings anywhere, as they are forced on third party users of code that itself isn't deprecating anything. Since Christian's tool is as I take it well underway couldn't we just rely on this? Also since it actually *fixes* the state of a ZODB, instead of just giving a lot of warnings and then leaving the helpless developer with writing some scary custom upgrade code.
I don't think there was a consensus about that.
For ZODB objects I think its okay for now not to use deprecations and to use the upgrade tool, but for the imports of other things that were moved elsewhere, I still think we need to bug developers that they need to upgrade their code with deprecation warnings, so we can eventually remove old imports. Oh, and BTW, if we use the "zodb fix tool", it's also okay to raise deprecation warnings about ZODB objects, as it's really easy to fix with the magic tool and again, we can eventually remove the deprecated import and make our code more clean.
Let's discuss it once again :)
OK. I have argued that leaving BBB imports in place forever, without deprecation, is the best choice, because it leaves working code working, and takes *less* maintenance than the "deprecate, then remove" dance we have been doing. Deprecation warnings which get emitted in huge swaths every time the appserver starts de-sensitize folks to them pretty quickly, making the warnings worse than useless. Martijn hoped for a tool which would help developers find places in their code which used the BBB locations, in order to help them modernize. I would be fine with leaving artifacts in the code to help such a tool (e.g., some variation on the zope.deferredimport dance). Tres. - -- =================================================================== Tres Seaver +1 540-429-0999 tseaver@palladion.com Palladion Software "Excellence by Design" http://palladion.com -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFJjGmQ+gerLs4ltQ4RAsYtAKCHJJsC/jCyw/cB5dHSdG678CdfKQCfXDFD UtrRsrV0gpPF3eSJm2HkdAk= =6E/I -----END PGP SIGNATURE-----
Hi there, On Fri, Feb 6, 2009 at 5:47 PM, Tres Seaver <tseaver@palladion.com> wrote: [snip]
Martijn hoped for a tool which would help developers find places in their code which used the BBB locations, in order to help them modernize.
And to repeat, this tool was mostly created last week in the enhanced test runner. BBB markers are not needed. And the other tool we got is documentation, which got started this week. Regards, Martijn
Dan Korostelev wrote at 2009-2-6 14:10 +0300:
... I still think we need to bug developers that they need to upgrade their code with deprecation warnings, so we can eventually remove old imports.
When you abuse deprecation warnings for minor cosmetic issues you risk that deprecation warnings are silenced altogether -- making even serious deprecation warnings less effective as they should be....
... Let's discuss it once again :)
Tres examples have been good, haven't they? -- Dieter
participants (4)
-
Dan Korostelev -
Dieter Maurer -
Martijn Faassen -
Tres Seaver