Can't add terms to join

[LordFlashmeow]

There is no way to add terms to a join.

In the constructor, it says:

# Additional kwargs are passed on to the `add_term` method
        self._terms: List[Term] = []
        _ = [Term(k.replace('__', '.'), kwargs[k]) for k in kwargs]

But the terms are not added to any list, and there is no add_term method in Join.py.

The only available method is terms(), which appears to expect a list of terms, but offers no way to convert the strings to terms.

def terms(self, *args: Term) -> 'Join':
        """Apply the given list of terms to the join."""
        self._terms = list(args)
        return self

One option is to mimic Query.py and implement the same add_terms() and tweak the Join constructor so it accepts terms as kwargs.

On a side note, why is it self._terms for Join and self.terms for Query?

I just wanted to get some hear if there were some other thoughts before making a PR.

[LordFlashmeow]

After fiddling around some more, I've found that add_terms() doesn't handle search modifiers like <, >, !, etc. I threw together some code that generates a Term given the field and value with the correct modifier.

[leonhard-s]

I like your generate_term method to let users use modifiers in the kwarg-style terms. I think we should still expose a manual way of adding them (i.e. the missing add_term method), I was originally trying to avoid the modifier literals.

That said, considering how much shared functionality there is between Join and Query (adding/removing terms, adding joins, showing and hiding fields, ...), it might be worth moving these shared functions into a common base class down the line.

[LordFlashmeow]

That would be an interesting experiment. I might give it a shot and see if it makes things simpler or more complex. That's my biggest concern, really, is that things become too complicated to maintain and improve.

For better or for worse, the reason I was able to contribute to this project was because everything was simply laid out and I could follow what the code was doing instantly. The more we abstract the code, the harder it will for someone new to understand what is going on.

[LordFlashmeow]

Thoughts on what we should to about terms()? It doesn't seem particularly useful, since it expects a list of Terms. On the other hand, it does allow you to reset the list of terms or set it easily if you've already generated the list earlier in the code.

My first choice would be to get rid of it. self.terms is public, so it is possible to manually set the value. The name also conflicts with add_terms, and you would have to look at the documentation or source code to understand what each one does.

[leonhard-s]

My first choice would be to get rid of it.

I share that sentiment. As I mentioned before, I made some questionable decisions in the first go-around of this wrapper, such as making any query commands methods.

I am currently in the process of blocking out the new object model, and the Query/Join methods will also get their refactoring to make sure they play along nicely with the object-oriented interface. I will publish the branch as soon as I feel like it's on the right track so it can be discussed before continuing that route.