Re: [Catalyst] Why does $c-stats require -Debug flag?
On Fri, 2008-04-25 at 15:53 +0100, Matt S Trout wrote: There's no written standard currently; I'd love to see somebody take a crack at writing one but I'm not sure what would need to go in it. I've attached a draft based on some of our company procedures to show the sorts of things that need to be addressed. I've changed a few things to reflect some of the Catalyst conventions that I am aware of but it will need your input, particular w.r.t. any conventions from PBP that you disagree with. having this interesting discussion. Can we put a timescale on it? What is the plan for release of 5.7013 and/or 5.80? Next week or two would be ideal; if you can't make time that soon then you need to say -now- so somebody else can fix this. I'd need 2-3 weeks as the next week and a half is out and I'm concerned about the time it will take to review the original code to check the subtleties, and then write new tests. The code itself is only a few minutes work... -- Jon SchutzMy tech notes http://notes.jschutz.net Chief Technology Officerhttp://www.youramigo.com YourAmigo (Proposed) Catalyst Coding Standards and Policies This document outlines the standards and policies for contributing to the Catalyst Project and attempts to capture the conventions that the project team has chosen to use to ensure that the quality and consistency of the code base is maintained. The project greatly benefits from the many contributors in the Catalyst community, and in turn the wider Catalyst community benefits from consistent and reliable code through adherence to these agreed standards. Any questions or clarifications regarding this document should be directed to the Catalyst mailing list. These standards are to be applied for core Catalyst development and are recommended for use with plugins and other contributed code. 1. Coding Standards 1.1. Perl Best Practices (Oreilly, Conway) should be followed unless otherwise noted here. 1.2. All code shall be clear of perlcritic warnings (at severity level 5), with the following exemptions: 1. Stricture disabled at line ... See page 429 of PBP. (Severity: 5) This warning is acceptable providing the 'no strict ...' is applied within the minimum scope necessary. (perlcritic does not seem to be able to reliably recognise minimum scope). 1.3 Indentation Use 4-space indentation. Hard tabs are prohibited. 2. Documentation Standards Please familiarise yourself with the Perl Best Practices description of types of documentation. 2.1 All User API functions shall have appropriate documentation suitable for end users of the API. These should appear in perldoc. 2.2 Documentation associated with functions and methods that are not part of the User API shall be as follows: - private functions and methods (as identified by a leading underscore in the function/method name) shall have any documentation in comments so as to not appear in perldoc. - internal functions and methods (meant for use within the same package only) shall be flagged as such and have documentation in comments so as to not appear in perldoc. - internal functions and methods that are not for general users buy may be used by extensions (which may be packaged separately, such as derived classes or plugins) should appear in perldoc, clearly identified as Developer API methods. 3. Testing Guidelines Write the documentation for your API functions to adequately define the inputs and expected outputs. Then write tests that match. * Each module should have at least one corresponding file in the 't/' test directory. Permissible exceptions: o where the module is trivial, such that it can be adequately tested by code inspection o where the module requires external infrastructure (e.g. a remote website) which is unreasonable to duplicate; in this case it may be more appropriate to add a working example into the 'examples/' subdirectory. Make sure that everything that can be reasonably tested through a regression test, is; this might mean splitting the module, placing the reusable, testable components in their own modules and keeping the application level code that requires the external infrastructure separate. o modules which only include configuration data for a suitably tested parent class may not need their own tests * All public methods (user and developer API methods) should be tested. Permissible exceptions: o where the method is inherited and suitably tested in the parent class o where the method is created through configuration of a suitably tested parent (such as accessors created using Class::Data::Inheritable or Class::Accessor) * Testing of private/protected methods may be appropriate when sufficient test coverage is difficult to achieve when testing
Re: [Catalyst] Why does $c-stats require -Debug flag?
On Mon, 2008-04-21 at 19:18 +0100, Matt S Trout wrote: On Mon, Apr 21, 2008 at 11:29:56AM +0930, Jon Schutz wrote: I'm making a stand here for the rights of all developers! Backward compatibility is a must for defined interfaces, but to carry that through to say that all design decisions turn into interfaces that must be preserved, even though not meant for external consumption, discourages innovation. Many factors separate good projects from bad, and one is well defined interfaces! And the tradition in perl is that if it doesn't start with an _, it's a public interface. The leading underscore indicates a private variable or function, from which it does _not_ follow that everything else is public. Example - Catalyst.pm contains: 1. Clearly private, internal methods with leading underscore and documentation in comments 2. Private methods with leading underscore that do pop up in the perldoc ($c-_localize_fields, which I presume is probably just an oversight) 3. Perldoc-documented methods with no leading underscore that are identified as internal methods (like 'friend' or 'protected' methods, I guess) - the doc says These methods are not meant to be used by end users. 4. Perldoc-documented methods with no leading underscore that are bona- fide user API methods. 5. Undocumented methods with no leading underscore ($c-setup_finished, for example). The point is, it's pretty clear what users _are_ allowed to access, so anyone applying the leading underscore rule does so at their peril. DBIx::Class originally used an if it's not documented it's not a public interface approach and I got massive negative feedback over that from people who'd followed the perl convention and got bitten later. But presumably that was because there was insufficient documentation in the first place, so early adopters had to get into the code. Which leads me to an aside - Why do we insist on having unit tests but let basic API documentation escape? How can we write tests without the API documentation?? Really, we are only having this discussion because $c-stats had no docs in the first place. Fact is I agree with most of what you say, it's just a question of where the boundary lies. If I had a choice, I'd follow documentation means public, no docs means use at your own risk. But there's an established standard, and that isn't it, so we live with the world we have. You're not making a stand for anything except your right to buck standard perl practice and get away with it; I tried, failed, had unhappy users, and learned. Welcome to the real world sucking. If it was perfect, we'd need a lot less developers :) Not at all; the implication of what you say is that standard perl practice is poor software engineering practice (not to say that there aren't inadequacies, of course there are). Respecting APIs is pretty fundamental, yes? -- Jon SchutzMy tech notes http://notes.jschutz.net Chief Technology Officerhttp://www.youramigo.com YourAmigo ___ List: Catalyst@lists.scsys.co.uk Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/ Dev site: http://dev.catalyst.perl.org/
Re: [Catalyst] Why does $c-stats require -Debug flag?
On Mon, 2008-04-21 at 19:16 +0100, Matt S Trout wrote:
On Mon, Apr 21, 2008 at 11:49:56AM +0930, Jon Schutz wrote:
On Sun, 2008-04-20 at 15:15 +0100, Matt S Trout wrote:
So far as I can see, all we really need to do is supply a proxy of the
common Tree::Simple method from the C::Stats object through to
$self-{tree}
and we're done. That'll provide compatibility with obvious usages without
adding any significant compatibility overhead.
I was hoping Jon would do this because he was the original author of
C::Stats
and could see any subtleties that needed paying attention to that I've
missed.
I would have to review the pre-5.7012 code but from memory there were
some differences in when internal fields in the tree were set, so
returning $self-{tree} will stop crashes but there may be some side
effects, such as inaccuracies in the resulting reports.
Well, if there is you can make the warning mention that.
Trouble with explicitly proxying the methods is that Tree::Simple has
many methods and who knows how many people have used what out there (I
suspect, very few and very little, but who knows?).
So? That's just a for() loop setting up *{$name} = sub { ... } entries.
You've heard my objections on principle and resistance due to minimal
residual impact in practice, but if one were to fix it, I suppose a
simple compromise would be something like this (untested):
That's not a compromise, that's an AUTOLOAD, which is poor coding practice
when you know what methods the object on the other side exists.
Indeed it is a compromise. PBP says don't use AUTOLOAD, but for all the
reasons it gives for not using it, it could probably have a footnote
saying something like If you're just putting in AUTOLOAD to support a
deprecated interface that's not going to be supported in the next major
revision, and the lifetime is pretty short, and nobody you know of is
actually using that deprecated interface anyway, then it's probably OK -
at least as a compromise. That's what it says in my copy of PBP, I
think.
I'm aware you object on principle; however I've stated very clearly why I
believe your objections are incorrect and since you're contributing to
Catalyst I'd ask that you follow the current Catalyst standards for
backwards compatibility even if you disagree, just the same as you'd do
for coding style and other matters of opinion. If I ever contribute to one
of your projects I'll happily return the favour :)
No problems, if that's what the Catalyst standard says; I must have
missed it. Where is it? I'd like to consult it on a number of
matters... please post the link.
Please can you do a specific setup, with tests; I'd suggest using
Class::Inspector to pull the list of methods and to proxy all those that
don't currently exist in your class.
Then we can have a warning included and happily throw these methods out in
5.80; the point is that people's code shouldn't go from fully working to
completely broken without a stage of still works but warns them they're
doing it wrong first (and note that if we'd called the method $c-_stats
I'd agree with you that it was private and we can deprecate it at will. but
we didn't. such is life)
The fact that it's supposedly already in a stage of completely broken
kind of undermines that theory.
I'm quite aware that I've spent more time debating the point than it
would have taken just to do this nugatory work, but then we wouldn't be
having this interesting discussion. Can we put a timescale on it? What
is the plan for release of 5.7013 and/or 5.80?
--
Jon SchutzMy tech notes http://notes.jschutz.net
Chief Technology Officerhttp://www.youramigo.com
YourAmigo
___
List: Catalyst@lists.scsys.co.uk
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/
Dev site: http://dev.catalyst.perl.org/
Re: [Catalyst] Why does $c-stats require -Debug flag?
* On Thu, Apr 24 2008, Jon Schutz wrote: On Mon, 2008-04-21 at 19:16 +0100, Matt S Trout wrote: That's not a compromise, that's an AUTOLOAD, which is poor coding practice when you know what methods the object on the other side exists. Indeed it is a compromise. It's not a compromise. It's laziness that makes Catalyst less reliable. Thousands of people rely on Catalyst to not take half-assed shortcuts. Why resort to hacks that have the potential to fuck things up for everyone but save 3 seconds of your time? This isn't PHP :) I'm aware you object on principle; however I've stated very clearly why I believe your objections are incorrect and since you're contributing to Catalyst I'd ask that you follow the current Catalyst standards for backwards compatibility even if you disagree, just the same as you'd do for coding style and other matters of opinion. If I ever contribute to one of your projects I'll happily return the favour :) No problems, if that's what the Catalyst standard says; I must have missed it. Where is it? I'd like to consult it on a number of matters... please post the link. Basically it's more of a zeitgeist than an actual document. There are some things that the community has decided and just do. One is not breaking things or adding features between point releases. We've fucked this up a number of times, but that doesn't really matter, the point is we try to fix our mistakes. Compare this to other frameworks that just break things and say fuck you. The fact that it's supposedly already in a stage of completely broken kind of undermines that theory. Not really. It just means we need to fix it even sooner. I'm quite aware that I've spent more time debating the point than it would have taken just to do this nugatory work, but then we wouldn't be having this interesting discussion. Can we put a timescale on it? What is the plan for release of 5.7013 and/or 5.80? Can you either: * do this now * or say you're not going to do it? That would make it easier for someone else to just get this done. Obviously you aren't obligated to do anything, because it's an open source project. But when someone contributes changes, we release them, and then realize that there's a problem, it's nice to have the contributor around to fix the issues. When they just disappear or argue against the project's conventions, it doesn't really look good, right? The stats code is good stuff. Why taint it with flamewars when it can be loved-by-everyone in just a few minutes? :) Regards, Jonathan Rockway -- print just = another = perl = hacker = if $,=$ ___ List: Catalyst@lists.scsys.co.uk Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/ Dev site: http://dev.catalyst.perl.org/
Re: [Catalyst] Why does $c-stats require -Debug flag?
On Thu, 2008-04-24 at 04:16 -0500, Jonathan Rockway wrote: No problems, if that's what the Catalyst standard says; I must have missed it. Where is it? I'd like to consult it on a number of matters... please post the link. Basically it's more of a zeitgeist than an actual document. There are some things that the community has decided and just do. One is not breaking things or adding features between point releases. We've fucked this up a number of times, but that doesn't really matter, the point is we try to fix our mistakes. Compare this to other frameworks that just break things and say fuck you. A standard is not a standard unless it's written down as a common reference for everybody to see. People in the community come and go and don't all have the same history, or longevity of memory for all the let's make this a standard decisions that happen along the way. This is perhaps getting close to the crux of the problem. Clearly Matt and I, and you it seems, have a different concept of what the standard is. Is there someone out there, then, with the right background, to set up a Wiki page and document this zeitgeist? The fact that it's supposedly already in a stage of completely broken kind of undermines that theory. Not really. It just means we need to fix it even sooner. I'm quite aware that I've spent more time debating the point than it would have taken just to do this nugatory work, but then we wouldn't be having this interesting discussion. Can we put a timescale on it? What is the plan for release of 5.7013 and/or 5.80? Can you either: * do this now * or say you're not going to do it? No I can't do it now, but may well be able to if given a time frame. That would make it easier for someone else to just get this done. Obviously you aren't obligated to do anything, because it's an open source project. But when someone contributes changes, we release them, and then realize that there's a problem, it's nice to have the contributor around to fix the issues. When they just disappear or argue against the project's conventions, it doesn't really look good, right? The stats code is good stuff. Why taint it with flamewars when it can be loved-by-everyone in just a few minutes? :) I thought we were having a discussion, an exchange of views, perhaps challenging what the conventions really are - and I think so far everyone contributing has managed to be fairly level about it. Apologies if my statements have been taken the wrong way. It seems to me that there are some underlying issues here which need to be sorted out. At least I think so. -- Jon SchutzMy tech notes http://notes.jschutz.net Chief Technology Officerhttp://www.youramigo.com YourAmigo ___ List: Catalyst@lists.scsys.co.uk Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/ Dev site: http://dev.catalyst.perl.org/
Re: [Catalyst] Why does $c-stats require -Debug flag?
On Apr 24, 2008, at 2:49 AM, Jon Schutz wrote: Basically it's more of a zeitgeist than an actual document. There are some things that the community has decided and just do. One is not breaking things or adding features between point releases. We've fucked this up a number of times, but that doesn't really matter, the point is we try to fix our mistakes. Compare this to other frameworks that just break things and say fuck you. A standard is not a standard unless it's written down as a common reference for everybody to see. People in the community come and go and don't all have the same history, or longevity of memory for all the let's make this a standard decisions that happen along the way. This is perhaps getting close to the crux of the problem. Clearly Matt and I, and you it seems, have a different concept of what the standard is. Is there someone out there, then, with the right background, to set up a Wiki page and document this zeitgeist? The leading underscore means private implicitly says no leading underscore means public. Just as a No Swimming at Night sign implies swimming during the day is allowed. Most of what Matt's talking about is etiquette. Unless something is clearly labeled alpha or beta, interface subject to change, it's just not okay to change an interface, hence backwards compatibility/support for at least the immediate, announced future. This seems an open-source-wide convention, not just Perl. Of course anyone can do whatever he wants with his code but breaking conventions (and this is a common sense one) won't make any friends and doing it enough will lead to abandonment or getting one's code forked away. I haven't used the stats stuff though it looks interesting, but I personally would immediately drop any package that went through an undocumented, unannounced interface change defended as personal style. Using AUTOLOAD when you know your methods/subs up front is a pointless complication and performance hit. -Ashley ___ List: Catalyst@lists.scsys.co.uk Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/ Dev site: http://dev.catalyst.perl.org/
Re: [Catalyst] Why does $c-stats require -Debug flag?
* On Thu, Apr 24 2008, Jon Schutz wrote: A standard is not a standard unless it's written down as a common reference for everybody to see. People in the community come and go and don't all have the same history, or longevity of memory for all the let's make this a standard decisions that happen along the way. This is perhaps getting close to the crux of the problem. Clearly Matt and I, and you it seems, have a different concept of what the standard is. Is there someone out there, then, with the right background, to set up a Wiki page and document this zeitgeist? I don't think anyone but you ever used the word standard. If it would make you feel better, let's define the standard to be whatever mst comes up with. He is the loudest, so he gets to decide. He also has that cool chainsaw thing. Anyway, this whole discussion sounds like you're being defensive. There is no need to be defensive. You didn't do anything wrong. When you sent the patch, you might not have known that we wanted to keep backcompat. I didn't know ;) And nobody told you that we wanted compat until recently. So it's not your fault for not initially adding the feature. But now we want it, and are anxiously awaiting a patch. You are most qualified to provide it. No I can't do it now, but may well be able to if given a time frame. You need to decide your own time frame. This is not work, it's an open source project. We are not going to tell you what to do. Someone will probably have to write it before the end of next week, though. This is an issue that we would like to resolve soon. It seems to me that there are some underlying issues here which need to be sorted out. At least I think so. If there are code issues, let's talk about them. If there is whining about community standards and so on, I am tired of talking about them. It's boring. You know how the old saying goes -- shut the fuck up and write some code :) Regards, Jonathan Rockway -- print just = another = perl = hacker = if $,=$ ___ List: Catalyst@lists.scsys.co.uk Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/ Dev site: http://dev.catalyst.perl.org/
Re: [Catalyst] Why does $c-stats require -Debug flag?
On Mon, Apr 21, 2008 at 11:49:56AM +0930, Jon Schutz wrote:
On Sun, 2008-04-20 at 15:15 +0100, Matt S Trout wrote:
So far as I can see, all we really need to do is supply a proxy of the
common Tree::Simple method from the C::Stats object through to $self-{tree}
and we're done. That'll provide compatibility with obvious usages without
adding any significant compatibility overhead.
I was hoping Jon would do this because he was the original author of
C::Stats
and could see any subtleties that needed paying attention to that I've
missed.
I would have to review the pre-5.7012 code but from memory there were
some differences in when internal fields in the tree were set, so
returning $self-{tree} will stop crashes but there may be some side
effects, such as inaccuracies in the resulting reports.
Well, if there is you can make the warning mention that.
Trouble with explicitly proxying the methods is that Tree::Simple has
many methods and who knows how many people have used what out there (I
suspect, very few and very little, but who knows?).
So? That's just a for() loop setting up *{$name} = sub { ... } entries.
You've heard my objections on principle and resistance due to minimal
residual impact in practice, but if one were to fix it, I suppose a
simple compromise would be something like this (untested):
That's not a compromise, that's an AUTOLOAD, which is poor coding practice
when you know what methods the object on the other side exists.
I'm aware you object on principle; however I've stated very clearly why I
believe your objections are incorrect and since you're contributing to
Catalyst I'd ask that you follow the current Catalyst standards for
backwards compatibility even if you disagree, just the same as you'd do
for coding style and other matters of opinion. If I ever contribute to one
of your projects I'll happily return the favour :)
Please can you do a specific setup, with tests; I'd suggest using
Class::Inspector to pull the list of methods and to proxy all those that
don't currently exist in your class.
Then we can have a warning included and happily throw these methods out in
5.80; the point is that people's code shouldn't go from fully working to
completely broken without a stage of still works but warns them they're
doing it wrong first (and note that if we'd called the method $c-_stats
I'd agree with you that it was private and we can deprecate it at will. but
we didn't. such is life)
--
Matt S Trout Need help with your Catalyst or DBIx::Class project?
Technical Directorhttp://www.shadowcat.co.uk/catalyst/
Shadowcat Systems Ltd. Want a managed development or deployment platform?
http://chainsawblues.vox.com/http://www.shadowcat.co.uk/servers/
___
List: Catalyst@lists.scsys.co.uk
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/
Dev site: http://dev.catalyst.perl.org/
Re: [Catalyst] Why does $c-stats require -Debug flag?
On Mon, Apr 21, 2008 at 11:29:56AM +0930, Jon Schutz wrote: I'm making a stand here for the rights of all developers! Backward compatibility is a must for defined interfaces, but to carry that through to say that all design decisions turn into interfaces that must be preserved, even though not meant for external consumption, discourages innovation. Many factors separate good projects from bad, and one is well defined interfaces! And the tradition in perl is that if it doesn't start with an _, it's a public interface. DBIx::Class originally used an if it's not documented it's not a public interface approach and I got massive negative feedback over that from people who'd followed the perl convention and got bitten later. If I had a choice, I'd follow documentation means public, no docs means use at your own risk. But there's an established standard, and that isn't it, so we live with the world we have. You're not making a stand for anything except your right to buck standard perl practice and get away with it; I tried, failed, had unhappy users, and learned. Welcome to the real world sucking. If it was perfect, we'd need a lot less developers :) -- Matt S Trout Need help with your Catalyst or DBIx::Class project? Technical Directorhttp://www.shadowcat.co.uk/catalyst/ Shadowcat Systems Ltd. Want a managed development or deployment platform? http://chainsawblues.vox.com/http://www.shadowcat.co.uk/servers/ ___ List: Catalyst@lists.scsys.co.uk Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/ Dev site: http://dev.catalyst.perl.org/
Re: [Catalyst] Why does $c-stats require -Debug flag?
On Sun, 2008-04-20 at 15:15 +0100, Matt S Trout wrote:
So far as I can see, all we really need to do is supply a proxy of the
common Tree::Simple method from the C::Stats object through to $self-{tree}
and we're done. That'll provide compatibility with obvious usages without
adding any significant compatibility overhead.
I was hoping Jon would do this because he was the original author of C::Stats
and could see any subtleties that needed paying attention to that I've
missed.
I would have to review the pre-5.7012 code but from memory there were
some differences in when internal fields in the tree were set, so
returning $self-{tree} will stop crashes but there may be some side
effects, such as inaccuracies in the resulting reports.
Trouble with explicitly proxying the methods is that Tree::Simple has
many methods and who knows how many people have used what out there (I
suspect, very few and very little, but who knows?).
You've heard my objections on principle and resistance due to minimal
residual impact in practice, but if one were to fix it, I suppose a
simple compromise would be something like this (untested):
our $AUTOLOAD;
sub AUTOLOAD {
my $self = shift;
my $name = $AUTOLOAD;
$name =~ s/.*://o;
warn Catalyst::Stats method $name is deprecated.;
return $self-{tree}-$name(@_);
}
--
Jon SchutzMy tech notes http://notes.jschutz.net
Chief Technology Officerhttp://www.youramigo.com
YourAmigo
___
List: Catalyst@lists.scsys.co.uk
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/
Dev site: http://dev.catalyst.perl.org/
Re: [Catalyst] Why does $c-stats require -Debug flag?
On Fri, 2008-04-18 at 19:54 +0100, Matt S Trout wrote: On Sat, Apr 12, 2008 at 08:37:49PM +0930, Jon Schutz wrote: Prior to 5.7012, $c-stats was an internal object (in so far as it was not documented as part of the API) so anyone manipulating it directly does so at their own risk. This one user who was kind enough to report it to the list will by far not be the only one with this problem. Catalyst::Plugin::SubRequest was also hit by this. Never, ever assume that 'undocumented' means 'I can break compat any time I like'. This works in theory, and in theory, theory is the same as practice, but ... I apologise to anyone who was inconvenienced by this change. I've written a guide to upgrading for anyone who was using the Tree::Simple object directly: http://notes.jschutz.net/17/perl/adding-action-timings-to-your-catalyst-output Nevertheless I stand by my comment that anyone who digs through the source code to find an internal object or function and then chooses to use that in external code does so at their own risk. Furthermore I suggest that anyone who is savvy enough to get into the code, understand it, and chooses to use an internal object/function, is aware of the risk and is more than capable of adapting their own code should an internal object/function change. Catalyst::Stats was introduced to define an interface for the stats object so that one could safely override the default implementation if they wished; the default implementation also introduced profiling methods that were not there before. You need to consider your failure to maintain backwards compatibility a bug and fix it. I said this at the time but apparently you didn't get round to fixing the Catalyst::Stats patch. Please do so as soon as you get time; Catalyst::Stats' API is a great leap forward but not at the cost of previously running code (although I think perhaps the compat code -should- warn that you're using an API you shouldn't have been and that it'll go away in the future). How would one define backwards compatibility in this case? That $c- stats must return a Tree::Simple object with all of the same user- defined fields as before? The logical extension of this argument says that on any minor release, the internal representation of *any* stored data or the signature of *any* internal function or method may not change, which surely is too restrictive on developers. 5.7012 has been out since December 2007. It seems to me a case of closing the gate after the horse has bolted. I don't hear an angry mob out there complaining. Perhaps as a result of this being raised the angry mob will come knocking; that being the case, I'll be happy to revisit the code. As it stands I'm unconvinced of the need for backward compatibility in this case, both of the principle (of preserving unpublished undocumented undefined interfaces) and the level of popular demand. -- Jon SchutzMy tech notes http://notes.jschutz.net Chief Technology Officerhttp://www.youramigo.com YourAmigo ___ List: Catalyst@lists.scsys.co.uk Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/ Dev site: http://dev.catalyst.perl.org/
Re: [Catalyst] Why does $c-stats require -Debug flag?
On Sat, Apr 12, 2008 at 08:37:49PM +0930, Jon Schutz wrote: Prior to 5.7012, $c-stats was an internal object (in so far as it was not documented as part of the API) so anyone manipulating it directly does so at their own risk. This one user who was kind enough to report it to the list will by far not be the only one with this problem. Catalyst::Plugin::SubRequest was also hit by this. Never, ever assume that 'undocumented' means 'I can break compat any time I like'. This works in theory, and in theory, theory is the same as practice, but ... Catalyst::Stats was introduced to define an interface for the stats object so that one could safely override the default implementation if they wished; the default implementation also introduced profiling methods that were not there before. You need to consider your failure to maintain backwards compatibility a bug and fix it. I said this at the time but apparently you didn't get round to fixing the Catalyst::Stats patch. Please do so as soon as you get time; Catalyst::Stats' API is a great leap forward but not at the cost of previously running code (although I think perhaps the compat code -should- warn that you're using an API you shouldn't have been and that it'll go away in the future). In future, if you intend to submit a patch which does not maintain compat, submit it against the next major release (currently 5.80), -not- the current maintainance release. Catalyst is meant to be production quality code with production quality stability, and sometimes that means maintaining compat with things not because people -should- be using them in the wild but because they -are- and we should always try to not break code over a point release. I'm aware this sucks, and I'm still very pleased we have a proper documented stats API, but it still needs doing. -- Matt S Trout Need help with your Catalyst or DBIx::Class project? Technical Directorhttp://www.shadowcat.co.uk/catalyst/ Shadowcat Systems Ltd. Want a managed development or deployment platform? http://chainsawblues.vox.com/http://www.shadowcat.co.uk/servers/ ___ List: Catalyst@lists.scsys.co.uk Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/ Dev site: http://dev.catalyst.perl.org/
Re: [Catalyst] Why does $c-stats require -Debug flag?
On Mon, 2008-04-14 at 20:21 +0100, Richard Jones wrote: Jon Schutz wrote: Did you set the -Stats flag to a non-zero value? i.e. use Catalyst qw/-Stats=1/ Err, no ;-) But I have now and it's fine - thanks. But there seems to be nothing I can do to set the flag at the command line when I start the devel. server (eg myapp_server.pl -Stats, myapp_server.pl MYAPP_STATS=1, myapp_server -STATS=1, etc). The idea is to enable stats for browser access but spare the t/*.t tests from dumping masses of timing data to the console when run. Other way around ... MYAPP_STATS=1 myapp_server.pl or export MYAPP_STATS=1 myapp_server.pl -- Jon SchutzMy tech notes http://notes.jschutz.net Chief Technology Officerhttp://www.youramigo.com YourAmigo ___ List: Catalyst@lists.scsys.co.uk Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/ Dev site: http://dev.catalyst.perl.org/
Re: [Catalyst] Why does $c-stats require -Debug flag?
On Apr 14, 2008, at 3:21 PM, Richard Jones wrote:
Jon Schutz wrote:
Did you set the -Stats flag to a non-zero value? i.e.
use Catalyst qw/-Stats=1/
Err, no ;-) But I have now and it's fine - thanks. But there seems
to be nothing I can do to set the flag at the command line when I
start the devel. server (eg myapp_server.pl -Stats, myapp_server.pl
MYAPP_STATS=1, myapp_server -STATS=1, etc). The idea is to enable
stats for browser access but spare the t/*.t tests from dumping
masses of timing data to the console when run.
I do it like this...
use Catalyst (
( $ENV{ Catalyst::Utils::class2env( __PACKAGE__ ).'_STATS' } ? ( -
Stats=1 ) : () ),
qw( Other Plugins Here ... ),
);
--
Jason Kohles, RHCA RHCDS RHCE
[EMAIL PROTECTED] - http://www.jasonkohles.com/
A witty saying proves nothing. -- Voltaire
___
List: Catalyst@lists.scsys.co.uk
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/
Dev site: http://dev.catalyst.perl.org/
Re: [Catalyst] Why does $c-stats require -Debug flag?
Jon Schutz wrote:
Following the example in the blog post, the solution is to set -Stats
and change your Root::end() method to something like:
@report = $stats-report;
$c-stash-{'action_stats'} = [EMAIL PROTECTED];
and then adjust your template as now you're getting an array of arrays
rather than an array of hashrefs.
Thanks, it's sort of working now, but I still need to run the server in
debug mode even with the -Stats flag, either by setting -Debug in my
main app.pm, or by starting the server with the -d flag, otherwise
$c-stats-report is undefined. Nearly there?
--
Richard Jones
___
List: Catalyst@lists.scsys.co.uk
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/
Dev site: http://dev.catalyst.perl.org/
Re: [Catalyst] Why does $c-stats require -Debug flag?
Jon Schutz wrote: On Fri, 2008-04-11 at 19:53 +0100, Richard Jones wrote: Yeah I tried -Stats already - something must have changed in a recent version of Catalyst::Stats, as now I get: [error] Caught exception in MyApp::Controller::Root-end Can't locate object method accept via package Catalyst::Stats at .. etc So it's now finding the stats method but apparently not accept. accept is a method of Tree::Simple I think. I didn't specifically load anything from Tree::Simple previously, it just worked using $c-stats-accept(). I think you probably want $c-stats-report, but can't think why you're calling this directly as it is invoked in finalize() if -Debug or -Stats is set. See perldoc Catalyst::Stats. Because I never got round to delving into Catalyst::Stats or Tree::Simple inner workings. I copy/pasted the example from http://www.onemogin.com/blog/559-adding-action-timings-to-your-output.html into my Root::end() method and it just worked. At least it did until I removed the -Debug flag. And then tried to recover functionality by updating Catalyst::Stats to the latest version, which also updated Catalyst itself (I was using an earlier 5.70x). So I've no idea in which module(s) the change occurred to stop the process working. Maybe someone more familiar with these modules could offer a clue? -- Richard Jones ___ List: Catalyst@lists.scsys.co.uk Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/ Dev site: http://dev.catalyst.perl.org/
Re: [Catalyst] Why does $c-stats require -Debug flag?
On Sat, 2008-04-12 at 10:45 +0100, Richard Jones wrote:
I think you probably want $c-stats-report, but can't think why you're
calling this directly as it is invoked in finalize() if -Debug or -Stats
is set. See perldoc Catalyst::Stats.
Because I never got round to delving into Catalyst::Stats or
Tree::Simple inner workings. I copy/pasted the example from
http://www.onemogin.com/blog/559-adding-action-timings-to-your-output.html
into my Root::end() method and it just worked.
At least it did until I removed the -Debug flag. And then tried to
recover functionality by updating Catalyst::Stats to the latest version,
which also updated Catalyst itself (I was using an earlier 5.70x). So
I've no idea in which module(s) the change occurred to stop the process
working. Maybe someone more familiar with these modules could offer a clue?
Following the example in the blog post, the solution is to set -Stats
and change your Root::end() method to something like:
@report = $stats-report;
$c-stash-{'action_stats'} = [EMAIL PROTECTED];
and then adjust your template as now you're getting an array of arrays
rather than an array of hashrefs.
Prior to 5.7012, $c-stats was an internal object (in so far as it was
not documented as part of the API) so anyone manipulating it directly
does so at their own risk. Catalyst::Stats was introduced to define an
interface for the stats object so that one could safely override the
default implementation if they wished; the default implementation also
introduced profiling methods that were not there before.
--
Jon SchutzMy tech notes http://notes.jschutz.net
Chief Technology Officerhttp://www.youramigo.com
YourAmigo
___
List: Catalyst@lists.scsys.co.uk
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/
Dev site: http://dev.catalyst.perl.org/
Re: [Catalyst] Why does $c-stats require -Debug flag?
Jon Schutz wrote: From the perldoc for 5.7012 - Stats collection is enabled when the '-Stats' options is set, debug is on or when the MYAPP_STATS environment variable is set. So, if you want stats but not debug, use -Stats instead of -Debug. Yeah I tried -Stats already - something must have changed in a recent version of Catalyst::Stats, as now I get: [error] Caught exception in MyApp::Controller::Root-end Can't locate object method accept via package Catalyst::Stats at .. etc So it's now finding the stats method but apparently not accept. accept is a method of Tree::Simple I think. I didn't specifically load anything from Tree::Simple previously, it just worked using $c-stats-accept(). -- Richard Jones ___ List: Catalyst@lists.scsys.co.uk Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/ Dev site: http://dev.catalyst.perl.org/
Re: [Catalyst] Why does $c-stats require -Debug flag?
On Thu, Apr 10, 2008 at 05:02:26PM +0100, Richard Jones wrote:
I've just tried to run my app. without the -Debug flag and now I just
get the '(en) Please come back later' (+ several other languages) error
page. I've traced it to the use of $c-stats in the end() method in the
Root controller, which is an ActionClass('RenderView') method:
[error] Caught exception in MyApp::Controller::Root-end Can't call
method accept on an undefined value at ... etc
$c-stats-accept() is used in generating script timing reports (Cory
Watson: Adding Action Timings To Your Output), and works fine if -Debug
is enabled.
Nothing else seems adversely affected by removing -Debug.
Catalyst::Stats is up to date, and the docs suggest it should
automatically be accessible via $c-stats. Any ideas anyone?
Debug mode is what turns on the internal statistics and action timing code,
so the stats object isn't initialised without it.
I'm not sure if this is a bug or a feature.
--
Matt S Trout Need help with your Catalyst or DBIx::Class project?
Technical Directorhttp://www.shadowcat.co.uk/catalyst/
Shadowcat Systems Ltd. Want a managed development or deployment platform?
http://chainsawblues.vox.com/http://www.shadowcat.co.uk/servers/
___
List: Catalyst@lists.scsys.co.uk
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/
Dev site: http://dev.catalyst.perl.org/
