Re: Issue 21 in addendum: Name collision with PHPUnit's "Target" class
adde...@googlecode.com
Comment #1 on issue 21 by shabbyrobe: Name collision with
PHPUnit's "Target" class
http://code.google.com/p/addendum/issues/detail?id=21
I have a crazy opinion on this. I don't reckon "Target" should be an
annotation - it's nice and neat, and a good illustration of exactly what
annotations are all about, but I think it's waaaay faster just to use
flags. For something that emulates a feature that should really be language
level, I think performance has to win over sweetness, although it _is_
kinda sweet using an annotation to annotate an annotation :)
I've been doing an awful lot of hacking on Addendum the last few days to
see what I can do to get the speed up - this was one area (apart from the
massive one that made almost all the difference - caching) that helped
quite a bit. I can sit down and do better numbers if you like.
I can see a few caveats that would potentially make this change undesirable:
* Firstly, it's a massive breaking change for anyone using the current
method of restricting target using the Target() annotation, although a few
stern warnings and a bump from 0.4 to 0.5 might mitigate that somewhat.
* I also have not looked in to how to test nested targets, though I
suppose adding another flag and passing it in in NestedAnnotationMatcher
would work
* Lots of doc updates would be needed too
My apologies if the below code ideas aren't perfect or even working - I'm
kind of going off the top of my head, but if there is any interest in going
down this road I will create a nice patch with proper tests (and hopefully
as few bogus "newline only" changes as possible!).
You have to tweak ReflectionAnnotated* a bit to have a $target property:
class ReflectionAnnotatedClass extends ReflectionClass {
public $target=Annotation::TARGET_CLASS;
// ...
}
class ReflectionAnnotatedMethod extends ReflectionMethod {
public $target=Annotation::TARGET_METHOD;
// ...
}
class ReflectionAnnotatedProperty extends ReflectionProperty {
public $target=Annotation::TARGET_PROPERTY;
// ...
}
Then change the calls to instantiateAnnotation that pass a target:
$this->instantiateAnnotation($class, $params,
$targetReflection->target);
And finally, add the extra bits to Annotation:
class Annotation {
const TARGET_METHOD=1;
const TARGET_CLASS=2;
const TARGET_PROPERTY=4;
const TARGET_ALL=7;
public static $targetNames = array(
self::TARGET_METHOD=>'method',
self::TARGET_CLASS=>'class',
self::TARGET_PROPERTY=>'property',
);
public function getTarget() {
return self::TARGET_ANY;
}
private function checkTargetConstraints($sourceTarget) {
if ($sourceTarget != null && !($sourceTarget &
$this->getTarget())) {
$type = self::$targetNames[$sourceTarget];
throw new Exception("Annotation '{$class}' is not valid on
target type '{$type}'");
}
}
}
So when you declare annotations, you just set the target using an OR:
class Pants extends Annotation {
public function getTarget() {
return self::TARGET_METHOD | self::TARGET_CLASS;
}
}
Oh, and there's also another way I just figured out for how to make the
changes to Annotation without wrecking the very good exception text created
in the current implementation of checkTargetConstraints - the Annotation
class itself can simply access $reflection->target, rather than it being
passed through to instantiateAnnotation.
adde...@googlecode.com
Comment #2 on issue 21 by shabbyrobe: Name collision with
http://code.google.com/p/addendum/issues/detail?id=21
Oh, I forgot to mention that the reason why I used a getTarget method and
not a property was that PHP doesn't let you use bitwise OR in property
declarations:
// Bzzzzt! Nope!
class Pants extends Annotation {
public $target = self::TARGET_METHOD | self::TARGET_CLASS;
}
Jan Suchal
first of all thanks for all the effort. You are the first one that
actually does patches that are directly applicable.
Now, any benchmarks on how much do we gain by this change? I think a
caching mechanism (adapter) in core that caches between requests would
solve this. I really do prefer cleaner and more elegant approaches when
speed in not an issue and I do think that this is a non-issue.
If you have any numbers - feel free to prove me wrong.
johno
adde...@googlecode.com
Comment #3 on issue 21 by shabbyrobe: Name collision with
http://code.google.com/p/addendum/issues/detail?id=21
Hang on, I just realised... wouldn't just using @Annotation_Target work?
adde...@googlecode.com
Comment #4 on issue 21 by ramon.poca: Name collision with
http://code.google.com/p/addendum/issues/detail?id=21
If you look at the issue date, class name was already changd to
Annotation_Target long ago.
About optimizations, aka the root of all evil, I'd leave it to the
instancing classes and keep Addendum doing one thing, doing it well and
keep being easy to read. ;)
shabbyrobe
On Jan 12, 1:50 am, adden...@googlecode.com wrote:
> Comment #4 on issue 21 by ramon.poca: Name collision with