Class methods can still be replaced outside the class #22510
Comments
That's not really the case, though. Reöpening a class means having to invalidate the constructor and field-init methods, because it might now gain more fields. Additionally, because fields are just weirdly scoped lexical variables, any newly-created methods in this "new scope but same class" wouldn't be able to see those existing fields. But it's all about the fields. As you point out, adding or replacing methods would be permitted via whatever MOP-like system exists (likely something in |
jkeenan
added
class
|
@leonerd wrote:
In a large code review, it's easy to miss a subtle |
|
@Ovid If you are missing things in your code review, please do adjust your set. I'm not sure anyone should be reviewing code in an environment when they don't have the time / space / legibility / attention to actually sit down and read the code being submitted. If the patch is too big either make the review longer, or the patch shorter. To me it feels like a longer syntax for replacing methods makes the problem of "a big patch" worse, because more code, no? |
|
@guest20 I hear where you're coming from and I wish that could work. In reality, people have deadlines, they get tired, they make mistakes.
That depends on the "longer syntax". Sure, you have to write more code to allow In the case of using sub replace_method ( $self, $sub_to_replace, $new_sub ) {
$self->_assert_fqname($sub_to_replace);
no strict 'refs';
$self->_maybe_cache(*$sub_to_replace{CODE});
no warnings 'redefine';
*$sub_to_replace = $new_sub;
return $self;
}You can't tell by visual inspection if that's correct or not. So let's rewrite it. sub replace_method ( $self, $sub_to_replace, $new_sub ) {
$self->_assert_fqname($sub_to_replace);
my ( $package, $subname ) = $sub_to_replace =~ /^(.*)::(.*)$/;
my $metapackage = meta::get_package($package);
$self->_maybe_cache($metapackage->get_symbol($subname));
$metapackage->add_symbol( $subname, $new_sub );
return $self;
}It's not much longer than the original version. In the above, we not only don't have to change the interface, but we no longer have to disable strict or warnings, and we're much more likely to have a correctly functioning code. If we're short on time or resources, better to write solid code that we can test and verify it works rather than rely on the human frailty of code reviews. I've been doing the latter for many years and things slip through the cracks all the time. And regarding my second version of that sub, did you see the bug in it? it won't run correctly, but unlike the first version, it will at least die instead of failing silently. |
|
These are certainly some excellent reasons why your code style guide, reviewer notes, etc.. should suggest the use of safer mechanisms like MOP/meta, over plain globref hackery. But by itself it doesn't seem a compelling reason why we should intentionally break the globref mechanism by adding special code to forbid it in these situations, purely for the reason of "we don't like it". |
|
So is the discussion about whether or not class methods should be allowed to be replaced outside the class or not, by means other than meta or MOP::Class (and currently they can be)...? I've recently begun following Perl on github, to keep me abreast of things, |
|
@Ovid If you don't have time to actually do code reviews, you're not actually reviewing the code. Just remove reviews from your work flow completely, because there's a high chance you're just approving based on author anyway. Pretending to do reviews takes much more time than not pretending to do reviews, so you'll have an even easier time of getting patches merged before your deadline. I don't know why you think that second version is easier to read. I don't know what any of those I do know what assigning a code-ref to
|
|
Re: PPC [ Proposed Perl Changes, PowerPC processor, Pay Per Click advertising... ] Ah..that's me...too much in the editorial world, and need a better head for advertising. ^_^. I like ovid's point, because it's metaphorically akin to fixing a leaky bucket. Of course, I don't normally use * (I am one of those people "allergic" to writing it) - except am currently using it to redefine a sub of Data Dumper, to get proper UTF-8 characters in the dumps ala https://stackoverflow.com/questions/50489062/ Presumably, if Data Dumper upgraded itself to use the class feature, AND redefining a method via a glob was made fatal, I'd have to update my glob line of code to use proper meta ways of doing the same thing. Of course, if Data Dumper ever got a UTF-8 output method, that might be preferable. Maybe there's a CPAN module that's a simple wrapper for Data Dumper and does that already, that I could look for? Anyway, anything of that ilk would all help make my code look less like a dirty hack copy and pasted from stack overflow, =). Because the code would look cleaner and I wouldn't feel I was resorting to hacks, I'm not nearly as advanced as Leonerd or Ovid when it comes to Perl. You are rolemodels and expert peers within the Perl community, and I am a mere mortal, trying to understand the thread, =) [and totally hyped for October's London Perl Workshop, where I can hopefully rub shoulders with such Perl experts and learn a thing or two]. P.S. I didn't search for perldoc meta - I simply saw meta::get_package in the code, so realised meta was a package name, and went to cpan.org to and typed in "meta" in the search box, and clicked on the result with 11+ good reviews, =). - https://metacpan.org/pod/meta - the POD then led me to: https://github.com/Perl/PPCs/blob/main/ppcs/ppc0022-metaprogramming.md |
|
@guest20 wrote:
Yes, that would be true if it's not "actually reviewing the code" and yes, I've seen "code reviews" that get rubber stamps based on author, so I get your point. But I've routinely found that even code reviews which miss stuff are generally not rubber stamps. We still get value out of them, including finding bugs and knowledge sharing. It's not perfect, but it's not worthless. And it's not a simple either/or, so saying "if it's not perfect, you shouldn't try" doesn't help. |
|
As an aside, while I'd be delighted if glob assignment syntax for methods ( |
Description
Classes cannot be redeclared. Declaring one more than once causes a compile time failure:
This was designed to prevent sloppy programmers rewriting classes without going through the MOP (which, to be fair, doesn't yet exist). However, it's still easy to workaround this issue, as shown in the next section.
Steps to Reproduce
And that prints:
We never see our
testvalue andtest3happens beforetest2due to control flow. It's a touch confusing and is the kind of bugs we'd like to avoid.Expected behavior
It would be nice if attempting to alter/replace any
method, other than using the MOP, was fatal. I recognize that classes and packages using the same namespace mechanism possibly makes this difficult to address.Perl configuration
The text was updated successfully, but these errors were encountered: