Metaxal / quickscript

Easy scripting for DrRacket
Other
17 stars 6 forks source link

add missing inner and super calls to augmented and overriden methods #78

Closed rfindler closed 11 months ago

rfindler commented 11 months ago

closes #77

Metaxal commented 11 months ago

Thanks for the fix!

Just checking: since these calls can be either before or after the body (and in the examples in the docs they are placed after the body), are you positive they are at the right place?

As a side note, to be honest, the docs for augment could be clearer about this. I could only find:

When a method is declared with pubment, augment, or overment, then a subclass augmenting method can be called using the inner form.

which doesn't really look like an imperative.

Metaxal commented 11 months ago

I'll assume you considered the order carefully. Let me know otherwise.

Is there a test I can add to my test cases?

rfindler commented 11 months ago

Just checking: since these calls can be either before or after the body (and in the examples in the docs they are placed after the body), are you positive they are at the right place?

I think it probably doesn't matter if they go before or after the calls to queue-callback. I suppose an inner call might raise an error and, in that case, you may or may not want to trigger quickscripts, but absent something along these lines, both are probably okay.

As a side note, to be honest, the docs for augment could be clearer about this. I could only find:

When a method is declared with pubment, augment, or overment, then a subclass augmenting method can be called using the inner form.

which doesn't really look like an imperative.

It isn't required to call inner in general. It is just that these methods are intended to be observers and are not intended to stop other observers, which is why calling inner is needed in this case. That is, it may be the case that some augmentable method wants to control what subclasses can see and so it would choose not to call inner in some (or all) situations. Here, however, that's not what we want.

rfindler commented 11 months ago

Is there a test I can add to my test cases?

I'm not seeing something obvious that can be added to tools. It may make sense for there to be a DrRacket test, however, that checks to make sure all tools actually invoke inner/super correctly by adding mixins at the top/bottom and then doing some action and then checking that the right methods got invoked. That'd have found this when the change was first made, which would have made it easier to figure out what was going wrong back in the original pr!

Metaxal commented 11 months ago

Ok, I see, thanks for the explanations!