Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No 'intellisense' autosuggestion of functions #372

Closed
corneliusroemer opened this issue Feb 9, 2021 · 10 comments
Closed

No 'intellisense' autosuggestion of functions #372

corneliusroemer opened this issue Feb 9, 2021 · 10 comments

Comments

@corneliusroemer
Copy link

I'm not sure if I've done something wrong installing TinyDB, but I'm not getting any intellisense/autocomplete, as I'm used to from most packages.

There also seem to be no type hints, maybe that's why? Is this something you'd want help with?

I'm using Pylance as my Language server in VS Code.

Here are screenshots from me using TinyDB in VSCode, note, return type is always none, so Pylance can't figure out what's going on.
image
image

@corneliusroemer

This comment has been minimized.

@corneliusroemer
Copy link
Author

Ok, I think I've figured out the problem.

TinyDB doesn't return a Table. Insert only gets hinted for Tables.

The quickstart examples are thus using a shortcut that harms type hinting.

Does TinyDB not always return a table? How could this be improved? Maybe the quickstart should include a call to table to make sure things are clear?

image

@msiemens
Copy link
Owner

TinyDB doesn't return a Table. Insert only gets hinted for Tables.

You're right on point! When I developed the first version of TinyDB back in 2013 (it's been a while!), static typing and intellisense wasn't really on my mind. The very first design didn't have tables, all methods like insert, search and update operated on the TinyDB object. But it quickly became clear to me that it would be convenient to allow to store data in different tables inside the same database file. Thus the concept of the default table was born. I implemented it by forwarding all unknown calls to TinyDB to the default table.

Thinking about this decision again, I see it as a trade-off between convenience and consistency: using TinyDB as a Table is very convenient, especially for newcomers, but it comes with the downside of a inferior experience when using type checking, especially considering the rising popularity of static analysis checkers like MyPy. The best solution would be to find a way that allows both. But to my knowledge it isn't possible to tell MyPy that TinyDB inherits all methods and properties of Table without being a subclass, although I need to do more research on that. Telling users to use tables to get intellisense support is a valid workaround, but I feel like there should be a better way than this.

I'll keep thinking about how we can find a better way for this.

@corneliusroemer
Copy link
Author

corneliusroemer commented Feb 13, 2021

Oh that makes sense, I wasn't aware the module was so old.

Your explanation makes sense. Having glanced through the code of TinyDB and Table I couldn't find a reason why TinyDB shouldn't be a subclass of Table.

The purpose of TinyDB seems twofold: First, it stores config options defining storage location etc. Second, it allows you to get, create and drop Table classes. To make TinyDB more user friendly you duck tape the methods of Table onto TinyDB through getattr forwarding.

tinydb/tinydb/database.py

Lines 246 to 253 in d142849

def __getattr__(self, name):
"""
Forward all unknown attribute calls to the default table instance.
"""
return getattr(self.table(self.default_table_name), name)
# Here we forward magic methods to the default table instance. These are
# not handled by __getattr__ so we need to forward them manually here

What would be the harm of treating TinyDB explicitly as a subclass of Table (with table name always being the default)? This would make the hacky treatment explicit. It'd look muddy, but at least it's explicit, whereas now the hackiness is quite obscure.

TinyDB works out of the box, without explicit table creation, yet if someone wants to, they can still create and use other tables by using the table creation and selection methods of TinyDB.

I'll try this out locally and see whether it works. Maybe there will be a surprise!

Is there a name for the getattr forwarding trick you're using or did you come up with this yourself? Is it a design pattern, maybe Facade?

@msiemens
Copy link
Owner

What would be the harm of treating TinyDB explicitly as a subclass of Table (with table name always being the default)?

Well, the very simple answer is that doing so breaks some tests 🙃

Is there a name for the getattr forwarding trick you're using or did you come up with this yourself? Is it a design pattern, maybe Facade?

To be honest, when I came up with the idea I didn't think very much about the theory behind it. In retrospect I guess it's the delegation pattern or delegation concept. Unfortunately delegation seems to be unspported by PEP 484 which defines the standard type hints.

@corneliusroemer
Copy link
Author

Right! Great, I think I've managed to rewrite it without breaking existing tests at all. Will submit PR in a minute.

@msiemens
Copy link
Owner

Hey @corneliusroemer. First, sorry for the long delay. I wanted to merge your PR in #374 for a while now but somehow I always felt hesitant when thinking about it. The main reason is that I felt like making TinyDB inherit Table is a really big change where I can't really foresee the backcompatibility implications. But I didn't have a better idea either…

Now I found a new approach that looks promising. The main goal after all is to get Visual Studio Code, MyPy and others to properly support TinyDB's API. And I found that we can achieve this by using typing.TYPE_CHEKING to make TinyDB work with Pylance (VS Code's Python Intellisense engine) and a MyPy plugin to make it work with MyPy.

Basically, my approach would be something like this:

def with_typehint(baseclass: Type[T]) -> Type[T]:
    if TYPE_CHECKING:
        return baseclass
    return object

TableBase = with_typehint(Table)

class TinyDB(TableBase):
    ...

This would make Pylance correctly detect that TinyDB somehow inherits from Table but at runtime it would just inherit from object like a plain Python class. Unfortunately, this is not supported by MyPy (see python/mypy#2813). But, as recommended in the linked issue, we can write a MyPy plugin that makes this pattern work with MyPy.

What do you think about this?

@corneliusroemer
Copy link
Author

Looks good! Should fix type hinting without disrupting anything else

@msiemens
Copy link
Owner

Great! I've pushed a commit that should add better type completion for PyCharm, VS Code and MyPy. Could you check whether you get better IntelliSense suggestions when using the development version of TinyDB (pip install git+https://github.com/msiemens/tinydb@master)?

@msiemens
Copy link
Owner

The fix has been released in TinyDB 4.5.0 which I've just published 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants