[terencehonles]

This change allows generating both MATERIALIZED and NOT MATERIALIZED queries.

[millerdev]

This does four things in one commit

• allows generating both MATERIALIZED and NOT MATERIALIZED queries
• deprecates name & materialized as positional arguments
• moves the default CTE name ("cte") into the initializer so callers can use None
• updates pytest to error on deprecation warnings.

I would prefer they had been done in separate commits.

deprecates name & materialized as positional arguments

Can you say a bit more about why this is necessary?

moves the default CTE name ("cte") into the initializer so callers can use None

Why would a caller want to use name=None?

[terencehonles]

Sorry I missed your comment @millerdev and have been pretty busy lately, but can respond and pick up this PR as appropriate.

I would prefer they had been done in separate commits.

I can look at the PR and see about splitting things apart. Let me know which things you're OK with and I can pull those out first to focus the rest of the discussion.

Can you say a bit more about why this is necessary?

This is not strictly necessary, but it makes it clear what the parameters are if you are using keyword arguments. If you don't want this I can just drop this change.

Why would a caller want to use name=None?

The caller doesn't really want no name, they just want to specify that the default name should be used. The way things are written, any calling code needs to encode some sort of default CTE name, but if this package just filled in None with the default CTE name the calling code could be simplified.

[millerdev]

Things I'd like to have split into separate commits:

• allows generating both MATERIALIZED and NOT MATERIALIZED queries
• updates pytest to error on deprecation warnings.

Things I'd like to drop:

• deprecates name & materialized as positional arguments
  • callers can use keyword arguments to make their code readable if they want to, but I don't think we need to force them.
• moves the default CTE name ("cte") into the initializer so callers can use None
  • if the caller doesn't want to specify a default then they should not pass name.

[terencehonles]

I will look at this later this next week. For the following:

moves the default CTE name ("cte") into the initializer so callers can use None

• if the caller doesn't want to specify a default then they should not pass name.

This doesn't really fix the issue since the caller, if it's a wrapper, needs to define a default name:

def fn(*, name='cte', **kwargs):  # every wrapper needs to copy the default name in its signature
    return CTE(queryset, name)

However, the wrapper may not care about the CTE name, and as written every wrapper needs to be defined with it. This just adds more boilerplate to CTE wrappers, and since all CTEs need a name this could be fixed by making the CTE code name or "cte" (even "" is invalid).

[millerdev]

This doesn't really fix the issue since the caller, if it's a wrapper, needs to define a default name:

def fn(*, name='cte', **kwargs):  # every wrapper needs to copy the default name in its signature
    return CTE(queryset, name)

Why is the wrapper discarding kwargs? Seems better to extract things the wrapper cares about and pass the remaining kwargs through:

def fn(*, wrapper_arg='whatever', **kwargs):
    # do something with wrapper_arg
    return CTE(queryset, **kwargs)

This seems rather hypothetical. Do you have a real example of where this is a problem?

[terencehonles]

Sorry, the point wasn't the **kwargs and yes, if all the keyword args are applicable to CTE then your example is fine.

Some reasons you might want to split out keyword only arguments out of **kwargs is:

• Your wrapper may use the kwargs too. You can use kwargs['name'] in the function, but you can also add it to the function signature as a keyword only argument. I'm not sure if we use this for the CTE code, but we do have this pattern.
• Adding type annotations to kwargs is "harder" and requires a TypedDict. PEP-764 (draft) or mypy's InlineTypedDict feature can change how these are declared, but all options are still wordy and the inline forms are only provisionally supported by type checkers.
• If the wrapper doesn't want to allow all of the keyword arguments it's still simpler to define an argument in its own signature.

I'll focus on splitting out the changes, and we can revisit this ^^^ later. I've moved this change to #150

[terencehonles]

It looks like GitHub's stacked PRs don't work across forks, so since this doesn't need #149 I'm rebasing the change again.

[millerdev]

I feel like this is trying hard to preserve functionality that likely no one cares about. If code exists anywhere that passes in the old default materialized=False, it will have a new meaning, and this will not detect it.

Rather than making this function more complex forever, what do you think of adding a BREAKING CHANGE note in the change log? We'll push a new major version for that, of course.

Feel free to push back if I'm missing a major reason why we need this.

[terencehonles]

There's not really a strong reason this is needed. I just noticed the __setstate__ method existed and it was keeping compatibility with its previous versions. I assumed pickling must have been a common use case. For us, this would mainly come up from django-rq, and that's how I noticed the issue that led me to #147.

I think the likelihood that we have a pickled CTE in the task queue is small, so while there is a non zero probability we can hit this, I think it's fine to just keep things simple, if that's what you prefer. Since the CTE queryset picking was already broken, as we have seen in #147, it's likely that pickling is not very common. Also, I believe this may have affected self.query which would break a CTE pickle too.

If we don't want to keep backward compatibility we may want to also simplify __getstate__ so it only handles 4 items and removes that precedence.

If you think the changelog note should be louder then, that's fine, but I assumed most people who use the argument are setting it to True if they need it or just not specifying it.

[millerdev]

Let's keep it simple and remove this.

I think the change log note does not need to be louder since pickle was already broken.

[terencehonles]

Done!

[millerdev]

Why is this change included in this PR? It doesn't seem related. Maybe could have been done in #147? I suppose it's fine to keep it here since it will likely be released in the same version as #147.

Edit: maybe it should be reverted since it will conflict with #134?

[terencehonles]

I included this change since you didn't want to preserve the backwards compat, and this was the code that encouraged me to keep it. I figured this change would be released with #147, so it would make sense to break it at the point that pickle was broken and I think the change should probably should go in. #134 should also not have the backwards compat it's adding, so the conflict resolution would be just to use its final else clause.

If you disagree, I can remove the change.

[millerdev]

Fair enough. Removing all backwards compatibility handling was not what I had intended, but I suppose as long as it's released with #147 it will not break anything that wasn't already broken (for other reasons).