datafolklabs / cement

Application Framework for Python
http://builtoncement.com
BSD 3-Clause "New" or "Revised" License
1.25k stars 117 forks source link

App.extend() not type hint friendly? #599

Open tomekr opened 3 years ago

tomekr commented 3 years ago

This may be an incorrect usage of App.extend() but when my CLI starts it creates an instance of a github client that gets used in a Github Cement controller:

app.extend("github", Github(access_token))

With this method of extending, I beleive you lose access for python's type hints to be aware of what app.github is and therefore you lose things like autocompletion in VS Code.

Possibly there is a better way to handle this use case?

derks commented 3 years ago

@tomekr that is the right usage os App.extend(), however the use case for it was always very loose. I added it as a means of tacking anything on to the app object (which get's passed around and used all over the place)... so if a plugin extends with .github I can also access app.github in another plugin.

It's probably one of the more un-pythonic things in Cement and open to any suggestions on a better implementation.

That said, do you have the same auto-complete issues with handler's? For example, does it auto-complete on app.config, app.log, etc? If we're good there, then it might be a matter of just creating your own interface and handler to implement it rather than .extend(). Let me know and I can help with setting up the interface/handler to see if that works.

tomekr commented 3 years ago

Yea it looks like self.app.* gets lost as well. This is a screenshot from a newly generated todo tutorial app:

todo

Back in the top-level main.py it looks like app is an instance of MyApp but the type system can't figure out the handlers.

toplevel

derks commented 3 years ago

I guess this is because we set app.log = None in App.__init__() and then later set it up during App.lay_cement() (setup). Maybe a downfall of having such a dynamic framework, or maybe I completely botched the design.

Any feedback, ideas, solutions here would be greatly appreciated.

richardj-bsquare commented 3 years ago

In Python 3.5+ you can use import typing to solve this, something along the lines of:

import typing.Optional

log: Optional[LoggingHandler] = None

Currently, so I can see the wood for the trees I usually assign logging to a variable like this using a type hint comment:

log: LoggingLogHandler = self.app.log # type: ignore

This neatly ignores the self.app.log (None) issue but is a pain having to set up everywhere.

richardj-bsquare commented 3 years ago

https://docs.python.org/3/library/typing.html

derks commented 3 years ago

@richardj-bsquare very helpful info. Will look at testing this and see what we can do to improve.

derks commented 3 years ago

@richardj-bsquare can you provide a MCVE of a CementApp doing typing like this?

coccoinomane commented 2 years ago

Any feedback, ideas, solutions here would be greatly appreciated.

Hi @derks !

As a workaround, what about casting self.app.* to App? It works when done in the controller class:

Markup 2022-10-31 at 17 10 08

Code completion works up to the first level, meaning that app.self.* is discovered, but app.self.config.* isn't. This could be solved:

  1. by also casting app's attributes (eg. app.self.config and app.self.config.cache) to their type, but only after these attributes have been set; or
  2. by defining getter functions in the controller with an explict return type, for example:

    from cement.core.config import ConfigInterface
    from cement.core.cache import CacheInterface
    
    def get_config(self) -> ConfigInterface:
       return self.app.config
    
    def get_cache(self) -> CacheInterface:
       return self.app.cache

Cheers, Cocco

PS: Please note that, in the screenshot, I missed a super().__init__() in the first line of the __init__() method.