issue_comments

Table actions

16 rows where issue = 1726236847

✎ View and edit SQL

This data as json, CSV (advanced)

Suggested facets: created_at (date), updated_at (date)

id ▼ html_url issue_url node_id user created_at updated_at author_association body reactions issue performed_via_github_app
1563282327 https://github.com/simonw/datasette/issues/2078#issuecomment-1563282327 https://api.github.com/repos/simonw/datasette/issues/2078 IC_kwDOBm6k_c5dLcuX simonw 9599 2023-05-25T17:46:05Z 2023-05-25T17:46:05Z OWNER Here's what `wrap_view()` does: https://github.com/simonw/datasette/blob/49184c569cd70efbda4f3f062afef3a34401d8d5/datasette/app.py#L1676-L1700 It's used e.g. here: https://github.com/simonw/datasette/blob/49184c569cd70efbda4f3f062afef3a34401d8d5/datasette/app.py#L1371-L1375 The `BaseView` thing meanwhile works like this: https://github.com/simonw/datasette/blob/d97e82df3c8a3f2e97038d7080167be9bb74a68d/datasette/views/base.py#L56-L157 {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} Resolve the difference between `wrap_view()` and `BaseView` 1726236847  
1563283939 https://github.com/simonw/datasette/issues/2078#issuecomment-1563283939 https://api.github.com/repos/simonw/datasette/issues/2078 IC_kwDOBm6k_c5dLdHj simonw 9599 2023-05-25T17:47:38Z 2023-05-25T17:47:38Z OWNER The idea behind `wrap_view()` is dependency injection - it's mainly used by plugins: https://docs.datasette.io/en/0.64.3/plugin_hooks.html#register-routes-datasette But I like the pattern so I started using it for some of Datasette's own features. I should use it for _all_ of Datasette's own features. But I still like the way `BaseView` helps with running different code for GET/POST/etc verbs. {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} Resolve the difference between `wrap_view()` and `BaseView` 1726236847  
1563292373 https://github.com/simonw/datasette/issues/2078#issuecomment-1563292373 https://api.github.com/repos/simonw/datasette/issues/2078 IC_kwDOBm6k_c5dLfLV simonw 9599 2023-05-25T17:55:12Z 2023-05-25T17:55:30Z OWNER So I think subclasses of `BaseView` need to offer a callable which accepts all five of the DI arguments - `datasette`, `request`, `scope`, `send`, `receive` - and then makes a decision based on the HTTP verb as to which method of the class to call. Those methods themselves can accept a subset of those parameters and will only be sent on to them. Having two layers of parameter detection feels a little bit untidy, but I think it will work. {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} Resolve the difference between `wrap_view()` and `BaseView` 1726236847  
1563294669 https://github.com/simonw/datasette/issues/2078#issuecomment-1563294669 https://api.github.com/repos/simonw/datasette/issues/2078 IC_kwDOBm6k_c5dLfvN simonw 9599 2023-05-25T17:57:06Z 2023-05-25T17:57:06Z OWNER I may need to be able to detect if a class instance has an `async def __call__` method - I think I can do that like so: ```python def iscoroutinefunction(obj): if inspect.iscoroutinefunction(obj): return True if hasattr(obj, '__call__') and inspect.iscoroutinefunction(obj.__call__): return True return False ``` From https://github.com/encode/starlette/issues/886#issuecomment-606585152 {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} Resolve the difference between `wrap_view()` and `BaseView` 1726236847  
1563308919 https://github.com/simonw/datasette/issues/2078#issuecomment-1563308919 https://api.github.com/repos/simonw/datasette/issues/2078 IC_kwDOBm6k_c5dLjN3 simonw 9599 2023-05-25T18:08:34Z 2023-05-25T18:08:34Z OWNER After much fiddling this seems to work: ```python import asyncio, types def is_async_callable(obj): if not callable(obj): raise ValueError("Object is not callable") if isinstance(obj, types.FunctionType): return asyncio.iscoroutinefunction(obj) if hasattr(obj, '__call__'): return asyncio.iscoroutinefunction(obj.__call__) raise ValueError("Not a function and has no __call__ attribute") ``` Tested like so: ```python class AsyncClass: async def __call__(self): pass class NotAsyncClass: def __call__(self): pass class ClassNoCall: pass async def async_func(): pass def non_async_func(): pass for thing in (AsyncClass(), NotAsyncClass(), ClassNoCall(), async_func, non_async_func): try: print(thing, is_async_callable(thing)) except Exception as ex: print(thing, ex) ``` ``` <__main__.AsyncClass object at 0x106c32150> True <__main__.NotAsyncClass object at 0x106c32390> False <__main__.ClassNoCall object at 0x106c32750> Object is not callable <function async_func at 0x1073d5120> True <function non_async_func at 0x1073d5080> False ``` {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} Resolve the difference between `wrap_view()` and `BaseView` 1726236847  
1563318598 https://github.com/simonw/datasette/issues/2078#issuecomment-1563318598 https://api.github.com/repos/simonw/datasette/issues/2078 IC_kwDOBm6k_c5dLllG simonw 9599 2023-05-25T18:17:03Z 2023-05-25T18:21:25Z OWNER I think I want that to return `(is_callable, is_async)` - so I can both test if the thing can be called AND if it should be awaited in the same operation (without any exceptions). I tried this: ```python def is_callable(obj): "Returns (is_callable, is_async_callable)" if not callable(obj): return False, False if isinstance(obj, types.FunctionType): return True, asyncio.iscoroutinefunction(obj) if hasattr(obj, '__call__'): return True, asyncio.iscoroutinefunction(obj.__call__) return False, False ``` ```python for thing in ( async_func, non_async_func, AsyncClass(), NotAsyncClass(), ClassNoCall(), AsyncClass, NotAsyncClass, ClassNoCall ): print(thing, is_callable(thing)) ``` And got: ``` <function async_func at 0x1073d5120> (True, True) <function non_async_func at 0x1073d5080> (True, False) <__main__.AsyncClass object at 0x106cce490> (True, True) <__main__.NotAsyncClass object at 0x106ccf710> (True, False) <__main__.ClassNoCall object at 0x106ccc810> (False, False) <class '__main__.AsyncClass'> (True, True) <class '__main__.NotAsyncClass'> (True, False) <class '__main__.ClassNoCall'> (True, False) ``` Which is almost right, but I don't like that `AsyncClass` is shown as callable (which it is, since it's a class) and awaitable (which it is not - the `__call__` method may be async but calling the class constructor is not). So I'm going to detect classes using `isinstance(obj, type)`. ```python def is_callable(obj): "Returns (is_callable, is_async_callable)" if not callable(obj): return False, False if isinstance(obj, type): # It's a class return True, False if isinstance(obj, types.FunctionType): return True, asyncio.iscoroutinefunction(obj) if hasattr(obj, '__call__'): return True, asyncio.iscoroutinefunction(obj.__call__) assert False, "obj {} somehow is callable with no __call__ method".format(obj) ``` I am re… {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} Resolve the difference between `wrap_view()` and `BaseView` 1726236847  
1563326000 https://github.com/simonw/datasette/issues/2078#issuecomment-1563326000 https://api.github.com/repos/simonw/datasette/issues/2078 IC_kwDOBm6k_c5dLnYw simonw 9599 2023-05-25T18:23:38Z 2023-05-25T18:23:38Z OWNER I don't like that `is_callable()` implies a single boolean result but actually returns a pair. I'll call it `check_callable(obj)` instead. {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} Resolve the difference between `wrap_view()` and `BaseView` 1726236847  
1563329245 https://github.com/simonw/datasette/issues/2078#issuecomment-1563329245 https://api.github.com/repos/simonw/datasette/issues/2078 IC_kwDOBm6k_c5dLoLd simonw 9599 2023-05-25T18:26:47Z 2023-05-25T18:28:08Z OWNER With type hints and a namedtuple: ```python import asyncio import types from typing import NamedTuple, Any class CallableStatus(NamedTuple): is_callable: bool is_async_callable: bool def check_callable(obj: Any) -> CallableStatus: if not callable(obj): return CallableStatus(False, False) if isinstance(obj, type): # It's a class return CallableStatus(True, False) if isinstance(obj, types.FunctionType): return CallableStatus(True, asyncio.iscoroutinefunction(obj)) if hasattr(obj, "__call__"): return CallableStatus(True, asyncio.iscoroutinefunction(obj.__call__)) assert False, "obj {} is somehow callable with no __call__ method".format(repr(obj)) ``` ```python for thing in ( async_func, non_async_func, AsyncClass(), NotAsyncClass(), ClassNoCall(), AsyncClass, NotAsyncClass, ClassNoCall, ): print(thing, check_callable(thing)) ``` ``` <function async_func at 0x1073d5120> CallableStatus(is_callable=True, is_async_callable=True) <function non_async_func at 0x1073d5080> CallableStatus(is_callable=True, is_async_callable=False) <__main__.AsyncClass object at 0x106ba7490> CallableStatus(is_callable=True, is_async_callable=True) <__main__.NotAsyncClass object at 0x106740150> CallableStatus(is_callable=True, is_async_callable=False) <__main__.ClassNoCall object at 0x10676d910> CallableStatus(is_callable=False, is_async_callable=False) <class '__main__.AsyncClass'> CallableStatus(is_callable=True, is_async_callable=False) <class '__main__.NotAsyncClass'> CallableStatus(is_callable=True, is_async_callable=False) <class '__main__.ClassNoCall'> CallableStatus(is_callable=True, is_async_callable=False) ``` {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} Resolve the difference between `wrap_view()` and `BaseView` 1726236847  
1563359114 https://github.com/simonw/datasette/issues/2078#issuecomment-1563359114 https://api.github.com/repos/simonw/datasette/issues/2078 IC_kwDOBm6k_c5dLveK simonw 9599 2023-05-25T18:47:57Z 2023-05-25T18:47:57Z OWNER Oops, that broke everything: ``` @documented async def await_me_maybe(value: typing.Any) -> typing.Any: "If value is callable, call it. If awaitable, await it. Otherwise return it." > if callable(value): E TypeError: 'module' object is not callable ``` {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} Resolve the difference between `wrap_view()` and `BaseView` 1726236847  
1563419066 https://github.com/simonw/datasette/issues/2078#issuecomment-1563419066 https://api.github.com/repos/simonw/datasette/issues/2078 IC_kwDOBm6k_c5dL-G6 simonw 9599 2023-05-25T19:42:16Z 2023-05-25T19:43:08Z OWNER Maybe what I want here is the ability to register classes with the router - and have the router know that if it's a class it should instantiate it via its constructor and then await `__call__` it. The neat thing about it is that it can reduce the risk of having a class instance that accidentally shares state between requests. It also encourages that each class only responds based on the `datasette, request, ...` objects that are passed to its methods. {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} Resolve the difference between `wrap_view()` and `BaseView` 1726236847  
1563444296 https://github.com/simonw/datasette/issues/2078#issuecomment-1563444296 https://api.github.com/repos/simonw/datasette/issues/2078 IC_kwDOBm6k_c5dMERI simonw 9599 2023-05-25T20:06:08Z 2023-05-25T20:06:08Z OWNER This prototype seems to work well: ```diff diff --git a/datasette/app.py b/datasette/app.py index d7dace67..ed0edf28 100644 --- a/datasette/app.py +++ b/datasette/app.py @@ -17,6 +17,7 @@ import secrets import sys import threading import time +import types import urllib.parse from concurrent import futures from pathlib import Path @@ -1266,6 +1267,8 @@ class Datasette: # TODO: /favicon.ico and /-/static/ deserve far-future cache expires add_route(favicon, "/favicon.ico") + add_route(wrap_view(DemoView, self), '/demo') + add_route( asgi_static(app_root / "datasette" / "static"), r"/-/static/(?P<path>.*)$" ) @@ -1673,8 +1676,46 @@ def _cleaner_task_str(task): return _cleaner_task_str_re.sub("", s) -def wrap_view(view_fn, datasette): - @functools.wraps(view_fn) +class DemoView: + async def __call__(self, datasette, request): + return Response.text("Hello there! {} - {}".format(datasette, request)) + +def wrap_view(view_fn_or_class, datasette): + is_function = isinstance(view_fn_or_class, types.FunctionType) + if is_function: + return wrap_view_function(view_fn_or_class, datasette) + else: + if not isinstance(view_fn_or_class, type): + raise ValueError("view_fn_or_class must be a function or a class") + return wrap_view_class(view_fn_or_class, datasette) + + +def wrap_view_class(view_class, datasette): + async def async_view_for_class(request, send): + instance = view_class() + if inspect.iscoroutinefunction(instance.__call__): + return await async_call_with_supported_arguments( + instance.__call__, + scope=request.scope, + receive=request.receive, + send=send, + request=request, + datasette=datasette, + ) + else: + return call_with_supported_arguments( + instance.__call__, … {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} Resolve the difference between `wrap_view()` and `BaseView` 1726236847  
1563488929 https://github.com/simonw/datasette/issues/2078#issuecomment-1563488929 https://api.github.com/repos/simonw/datasette/issues/2078 IC_kwDOBm6k_c5dMPKh simonw 9599 2023-05-25T20:48:12Z 2023-05-25T20:48:39Z OWNER Actually no need for that extra level of parameter detection: `BaseView.__call__` should _always_ take `datasette, request` - `scope` and `receive` are both available on `request`, and `send` is only needed if you're not planning on returning a `Response` object. So the `get` and `post` and suchlike methods should take `datasette` and `request` too. {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} Resolve the difference between `wrap_view()` and `BaseView` 1726236847  
1563498048 https://github.com/simonw/datasette/issues/2078#issuecomment-1563498048 https://api.github.com/repos/simonw/datasette/issues/2078 IC_kwDOBm6k_c5dMRZA simonw 9599 2023-05-25T20:57:52Z 2023-05-25T20:58:13Z OWNER Here's a new `BaseView` class that automatically populates `OPTIONS` based on available methods: ```python class BaseView: async def head(self, *args, **kwargs): try: response = await self.get(*args, **kwargs) response.body = b"" return response except AttributeError: raise async def method_not_allowed(self, request): if ( request.path.endswith(".json") or request.headers.get("content-type") == "application/json" ): response = Response.json( {"ok": False, "error": "Method not allowed"}, status=405 ) else: response = Response.text("Method not allowed", status=405) return response async def options(self, request, *args, **kwargs): response = Response.text("ok") response.headers["allow"] = ", ".join( method.upper() for method in ("head", "get", "post", "put", "patch", "delete") if hasattr(self, method) ) return response async def __call__(self, request, datasette): try: handler = getattr(self, request.method.lower()) return await handler(request, datasette) except AttributeError: return await self.method_not_allowed(request) class DemoView(BaseView): async def get(self, datasette, request): return Response.text("Hello there! {} - {}".format(datasette, request)) post = get ``` {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} Resolve the difference between `wrap_view()` and `BaseView` 1726236847  
1563511171 https://github.com/simonw/datasette/issues/2078#issuecomment-1563511171 https://api.github.com/repos/simonw/datasette/issues/2078 IC_kwDOBm6k_c5dMUmD simonw 9599 2023-05-25T21:11:20Z 2023-05-25T21:13:05Z OWNER I'm going to call this `VerbView` for the moment. Might even rename it to `View` later. {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} Resolve the difference between `wrap_view()` and `BaseView` 1726236847  
1563522011 https://github.com/simonw/datasette/issues/2078#issuecomment-1563522011 https://api.github.com/repos/simonw/datasette/issues/2078 IC_kwDOBm6k_c5dMXPb simonw 9599 2023-05-25T21:22:30Z 2023-05-25T21:22:30Z OWNER This is bad: ```python async def __call__(self, request, datasette): try: handler = getattr(self, request.method.lower()) return await handler(request, datasette) except AttributeError: return await self.method_not_allowed(request) ``` Because it hides any `AttributeError` exceptions that might occur in the view code. {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} Resolve the difference between `wrap_view()` and `BaseView` 1726236847  
1563625093 https://github.com/simonw/datasette/issues/2078#issuecomment-1563625093 https://api.github.com/repos/simonw/datasette/issues/2078 IC_kwDOBm6k_c5dMwaF simonw 9599 2023-05-25T23:23:15Z 2023-05-25T23:23:15Z OWNER Rest of the work on this will happen in the PR: - #2080 {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} Resolve the difference between `wrap_view()` and `BaseView` 1726236847  

Advanced export

JSON shape: default, array, newline-delimited, object

CSV options: 

CREATE TABLE [issue_comments] (
   [html_url] TEXT,
   [issue_url] TEXT,
   [id] INTEGER PRIMARY KEY,
   [node_id] TEXT,
   [user] INTEGER REFERENCES [users]([id]),
   [created_at] TEXT,
   [updated_at] TEXT,
   [author_association] TEXT,
   [body] TEXT,
   [reactions] TEXT,
   [issue] INTEGER REFERENCES [issues]([id])
, [performed_via_github_app] TEXT);
CREATE INDEX [idx_issue_comments_issue]
                ON [issue_comments] ([issue]);
CREATE INDEX [idx_issue_comments_user]
                ON [issue_comments] ([user]);