Move annotation logic into Query Runner

[arikfr]

What type of PR is this? (check all applicable)

• Refactor

Description

This is a small refactor towards allowing controlling the query annotation in the query runner (a relevant use case is coming up in a follow up PR).

In this pull request, we move the annotation logic into the query runner (with a default implementation in BaseQueryRunner).

[arikfr]

Nothing changed here, just auto formatting.

[rauchy]

What's the benefit in implementing this as a mixin, as opposed to an actual method on BaseQueryRunner?

[arikfr]

What's the benefit in implementing this as a mixin, as opposed to an actual method on BaseQueryRunner?

Because the one implemented in BaseQueryRunner is the implementation that does annotate, so we need the mixin to override it.

Although now I wonder if we might get bitten by the order in which the methods get overriden.

[rauchy]

Yeah, I got that, but still -

(a) seems like most query runners have no annotations (in this PR, you mix it in about 35 times).
(b) adding a mixin to remove functionality feels counterintuitive.

Don't you think it'll be easier to understand if it were the other way around? (an AnnotationMixin)

[arikfr]

seems like most query runners have no annotations (in this PR, you mix it in about 35 times).

You should improve your grep condition :-) You probably included the import statements in this count. It's being used 27 times. We have 56 Query Runners, so that's 48% ;-) So we still have a majority that use annotations.

I still prefer the annotation to be the default, but here's an alternative approach: add a kill switch instead of mixin, so it becomes:

class JSON(BaseHTTPQueryRunner):
    add_query_annotation = False

Might need a better name for the flag.

[rauchy]

You should improve your grep condition :-)

damnit you're right 😠 I still find a "mixins add behavior" approach simpler, but 🤷‍♂

Regarding the alternative - how about simply annotate = True.
Also, did you consider an @annotated decorator?

[arikfr]

I agree that it makes more sense for a mixin to add behavior. Although one could claim that this adds behavior too (of doing nothing). 🤪

Regarding the alternative - how about simply annotate = True.

Feels a bit unclear, like "annotate what"? Ideally it would be annotate_query, but that's the method name :-(

[rauchy]

Feels a bit unclear, like "annotate what"?

True dat.

Also, did you consider an @Annotated decorator?

[arikfr]

Also, did you consider an @Annotated decorator?

Feel like an overkill for something that can be a simple boolean class property.