github
html_url | issue_url | id | node_id | user | created_at | updated_at | author_association | body | reactions | issue | performed_via_github_app |
---|---|---|---|---|---|---|---|---|---|---|---|
https://github.com/simonw/datasette/pull/1648#issuecomment-1059823151 | https://api.github.com/repos/simonw/datasette/issues/1648 | 1059823151 | IC_kwDOBm6k_c4_K54v | 22429695 | 2022-03-05T19:56:41Z | 2022-03-07T15:38:08Z | NONE | # [Codecov](https://codecov.io/gh/simonw/datasette/pull/1648?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Simon+Willison) Report > Merging [#1648](https://codecov.io/gh/simonw/datasette/pull/1648?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Simon+Willison) (32548b8) into [main](https://codecov.io/gh/simonw/datasette/commit/7d24fd405f3c60e4c852c5d746c91aa2ba23cf5b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Simon+Willison) (7d24fd4) will **increase** coverage by `0.02%`. > The diff coverage is `100.00%`. [![Impacted file tree graph](https://codecov.io/gh/simonw/datasette/pull/1648/graphs/tree.svg?width=650&height=150&src=pr&token=eSahVY7kw1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Simon+Willison)](https://codecov.io/gh/simonw/datasette/pull/1648?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Simon+Willison) ```diff @@ Coverage Diff @@ ## main #1648 +/- ## ========================================== + Coverage 92.03% 92.05% +0.02% ========================================== Files 34 34 Lines 4557 4570 +13 ========================================== + Hits 4194 4207 +13 Misses 363 363 ``` | [Impacted Files](https://codecov.io/gh/simonw/datasette/pull/1648?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Simon+Willison) | Coverage Δ | | |---|---|---| | [datasette/url\_builder.py](https://codecov.io/gh/simonw/datasette/pull/1648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Simon+Willison#diff-ZGF0YXNldHRlL3VybF9idWlsZGVyLnB5) | `100.00% <100.00%> (ø)` | … | {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} | 1160432941 | |
https://github.com/simonw/datasette/issues/1654#issuecomment-1061197133 | https://api.github.com/repos/simonw/datasette/issues/1654 | 1061197133 | IC_kwDOBm6k_c4_QJVN | 9599 | 2022-03-07T22:19:35Z | 2022-03-07T22:19:35Z | OWNER | Also now live on https://datasette.io ![CleanShot 2022-03-07 at 14 18 30@2x](https://user-images.githubusercontent.com/9599/157127424-805b3166-f0a8-4fac-be87-c055740af580.png) | {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} | 1161969891 | |
https://github.com/simonw/datasette/issues/1650#issuecomment-1061041034 | https://api.github.com/repos/simonw/datasette/issues/1650 | 1061041034 | IC_kwDOBm6k_c4_PjOK | 9599 | 2022-03-07T19:16:51Z | 2022-03-07T19:16:51Z | OWNER | Here's the problem: https://github.com/simonw/datasette/blob/020effe47bf89f35182960a9645f2383a42ebd54/datasette/utils/__init__.py#L1173-L1175 Which is called here: https://github.com/simonw/datasette/blob/1baa030eca375f839f3471237547ab403523e643/datasette/views/base.py#L469-L473 So `table%2Fwith%2Fslashes` ends up decoded as if it was using dash encoding. | {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} | 1160750713 | |
https://github.com/simonw/datasette/issues/1651#issuecomment-1060853226 | https://api.github.com/repos/simonw/datasette/issues/1651 | 1060853226 | IC_kwDOBm6k_c4_O1Xq | 9599 | 2022-03-07T16:04:26Z | 2022-03-07T16:04:26Z | OWNER | Here's the relevant code: https://github.com/simonw/datasette/blob/1baa030eca375f839f3471237547ab403523e643/datasette/utils/__init__.py#L753-L772 https://github.com/simonw/datasette/blob/1baa030eca375f839f3471237547ab403523e643/datasette/views/base.py#L451-L479 | {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} | 1161584460 | |
https://github.com/simonw/datasette/issues/1439#issuecomment-1060870237 | https://api.github.com/repos/simonw/datasette/issues/1439 | 1060870237 | IC_kwDOBm6k_c4_O5hd | 9599 | 2022-03-07T16:19:22Z | 2022-03-07T16:19:22Z | OWNER | I didn't need to do any of the fancy regular expression routing stuff after all, since the new dash encoding format avoids using `/` so a simple `[^/]+` can capture the correct segments from the URL. | {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} | 973139047 | |
https://github.com/simonw/datasette/issues/1651#issuecomment-1061053094 | https://api.github.com/repos/simonw/datasette/issues/1651 | 1061053094 | IC_kwDOBm6k_c4_PmKm | 9599 | 2022-03-07T19:29:01Z | 2022-03-07T19:29:01Z | OWNER | I found an obscure bug in #1650 which I can fix with this too. The following test should pass: ```python @pytest.mark.parametrize( "path,expected", ( ( "/fivethirtyeight/twitter-ratio%2Fsenators", "/fivethirtyeight/twitter-2Dratio-2Fsenators", ), ( "/fixtures/table%2Fwith%2Fslashes.csv", "/fixtures/table-2Fwith-2Fslashes-2Ecsv", ), # query string should be preserved ("/foo/bar%2Fbaz?id=5", "/foo/bar-2Fbaz?id=5"), ), ) def test_redirect_percent_encoding_to_dash_encoding(app_client, path, expected): response = app_client.get(path) assert response.status == 302 assert response.headers["location"] == expected ``` It currently fails like this: ``` > assert response.headers["location"] == expected E AssertionError: assert '/fixtures/table-2Fwith-2Fslashes.csv?_nofacet=1&_nocount=1' == '/fixtures/table-2Fwith-2Fslashes-2Ecsv' E - /fixtures/table-2Fwith-2Fslashes-2Ecsv E + /fixtures/table-2Fwith-2Fslashes.csv?_nofacet=1&_nocount=1 ``` Because the logic in that `get_format()` function notices that the table exists, and then weird things happen here: https://github.com/simonw/datasette/blob/1baa030eca375f839f3471237547ab403523e643/datasette/views/base.py#L288-L303 | {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} | 1161584460 | |
https://github.com/simonw/datasette/issues/1654#issuecomment-1061184206 | https://api.github.com/repos/simonw/datasette/issues/1654 | 1061184206 | IC_kwDOBm6k_c4_QGLO | 9599 | 2022-03-07T22:04:51Z | 2022-03-07T22:04:51Z | OWNER | I'm going to add this to the main Datasette repo (done) and the `datasette.io` website too. | {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} | 1161969891 | |
https://github.com/simonw/datasette/issues/1651#issuecomment-1061223822 | https://api.github.com/repos/simonw/datasette/issues/1651 | 1061223822 | IC_kwDOBm6k_c4_QP2O | 9599 | 2022-03-07T22:54:54Z | 2022-03-07T22:54:54Z | OWNER | I'm going to do a review of how URL routing works at the moment for the various views. I edited down [the full list](https://github.com/simonw/datasette/blob/c5791156d92615f25696ba93dae5bb2dcc192c98/datasette/app.py#L997-L1107) a bit - these are the most relevant: ```python add_route(IndexView.as_view(self), r"/(?P<as_format>(\.jsono?)?$)") add_route( DatabaseView.as_view(self), r"/(?P<db_name>[^/]+?)(?P<as_format>" + renderer_regex + r"|.jsono|\.csv)?$", ) add_route( TableView.as_view(self), r"/(?P<db_name>[^/]+)/(?P<table_and_format>[^/]+?$)", ) add_route( RowView.as_view(self), r"/(?P<db_name>[^/]+)/(?P<table>[^/]+?)/(?P<pk_path>[^/]+?)(?P<as_format>" + renderer_regex + r")?$", ) ``` | {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} | 1161584460 | |
https://github.com/simonw/datasette/issues/1654#issuecomment-1061181089 | https://api.github.com/repos/simonw/datasette/issues/1654 | 1061181089 | IC_kwDOBm6k_c4_QFah | 9599 | 2022-03-07T22:01:38Z | 2022-03-07T22:01:38Z | OWNER | I'm going to use the [widely adopted](https://www.contributor-covenant.org/adopters/) Contributor Covenant: https://www.contributor-covenant.org/version/1/4/code-of-conduct/ | {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} | 1161969891 | |
https://github.com/simonw/datasette/issues/1651#issuecomment-1061170897 | https://api.github.com/repos/simonw/datasette/issues/1651 | 1061170897 | IC_kwDOBm6k_c4_QC7R | 9599 | 2022-03-07T21:48:35Z | 2022-03-07T21:48:35Z | OWNER | My attempts to simplify `get_format()` keep resulting in errors like this one: ``` File "/Users/simon/Dropbox/Development/datasette/datasette/views/base.py", line 474, in view_get response_or_template_contexts = await self.data( TypeError: TableView.data() missing 1 required positional argument: 'table' ``` I really need to clean this up. | {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} | 1161584460 | |
https://github.com/simonw/datasette/issues/1654#issuecomment-1061182132 | https://api.github.com/repos/simonw/datasette/issues/1654 | 1061182132 | IC_kwDOBm6k_c4_QFq0 | 9599 | 2022-03-07T22:02:43Z | 2022-03-07T22:02:43Z | OWNER | Neat, GitHub have a template for this https://github.com/simonw/datasette/community/code-of-conduct/new?template=contributor-covenant | {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} | 1161969891 | |
https://github.com/simonw/datasette/issues/1650#issuecomment-1060864823 | https://api.github.com/repos/simonw/datasette/issues/1650 | 1060864823 | IC_kwDOBm6k_c4_O4M3 | 9599 | 2022-03-07T16:14:33Z | 2022-03-07T16:14:33Z | OWNER | Same problem here: https://fivethirtyeight.datasettes.com/fivethirtyeight/ahca-2Dpolls%2Fahca_polls should redirect but doesn't. | {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} | 1160750713 | |
https://github.com/simonw/datasette/issues/1650#issuecomment-1061038414 | https://api.github.com/repos/simonw/datasette/issues/1650 | 1061038414 | IC_kwDOBm6k_c4_PilO | 9599 | 2022-03-07T19:14:04Z | 2022-03-07T19:14:04Z | OWNER | The problem seems to be that `http://127.0.0.1:8002/fixtures/table%2Fwith%2Fslashes.csv` doesn't result in a 404 at all. If it did, it would be redirected. | {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} | 1160750713 | |
https://github.com/simonw/datasette/issues/1653#issuecomment-1061150672 | https://api.github.com/repos/simonw/datasette/issues/1653 | 1061150672 | IC_kwDOBm6k_c4_P9_Q | 9599 | 2022-03-07T21:23:39Z | 2022-03-07T21:23:39Z | OWNER | There may be a short-term fix for this: table view could start accepting a `?_sort_sql=SQLfragment` parameter, similar to the `?_where=` parameter described here: https://docs.datasette.io/en/stable/json_api.html#special-table-arguments That fragment could then be pre-populated in `metadata`. Makes me think maybe that `?_where=` should be optionally settable in metadata too? | {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} | 1161937073 | |
https://github.com/simonw/datasette/issues/1650#issuecomment-1060863311 | https://api.github.com/repos/simonw/datasette/issues/1650 | 1060863311 | IC_kwDOBm6k_c4_O31P | 9599 | 2022-03-07T16:13:17Z | 2022-03-07T16:13:17Z | OWNER | This doesn't seem to work. https://latest.datasette.io/fixtures/table%2Fwith%2Fslashes.csv should be redirecting now that this is deployed - which it is, because https://latest.datasette.io/-/versions shows 644d25d1de78a36b105cca479e7b3e4375a6eadc - but I'm not getting that redirect. | {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} | 1160750713 | |
https://github.com/simonw/datasette/issues/1650#issuecomment-1060836262 | https://api.github.com/repos/simonw/datasette/issues/1650 | 1060836262 | IC_kwDOBm6k_c4_OxOm | 9599 | 2022-03-07T15:52:09Z | 2022-03-07T15:52:09Z | OWNER | This is a bit tricky. I tried this, sending a redirect only if a 404 happens: ```diff diff --git a/datasette/app.py b/datasette/app.py index 8c5480c..420664c 100644 --- a/datasette/app.py +++ b/datasette/app.py @@ -1211,6 +1211,10 @@ class DatasetteRouter: return await self.handle_404(request, send) async def handle_404(self, request, send, exception=None): + # If path contains % encoding, redirect to dash encoding + if '%' in request.scope["path"]: + await asgi_send_redirect(send, request.scope["path"].replace("%", "-")) + return # If URL has a trailing slash, redirect to URL without it path = request.scope.get( "raw_path", request.scope["path"].encode("utf8") ``` But this URL didn't work: - http://127.0.0.1:8001/fivethirtyeight/twitter-ratio%2Fsenators I was expecting that to redirect to this page: - http://127.0.0.1:8001/fivethirtyeight/twitter-2Dratio-2Fsenators But instead it took me to another 404: - http://127.0.0.1:8001/fivethirtyeight/twitter-ratio%2Fsenators This is because that URL contains both a %-escaped `/` AND a plain `-` - which was not escaped in the old system but is escaped in the new system. | {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} | 1160750713 | |
https://github.com/simonw/datasette/issues/1653#issuecomment-1061148807 | https://api.github.com/repos/simonw/datasette/issues/1653 | 1061148807 | IC_kwDOBm6k_c4_P9iH | 9599 | 2022-03-07T21:21:23Z | 2022-03-07T21:21:23Z | OWNER | This is currently blocked on the fact that Datasette doesn't have a mechanism for sorting by more than one column: - #197 | {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} | 1161937073 | |
https://github.com/simonw/datasette/issues/647#issuecomment-1061226942 | https://api.github.com/repos/simonw/datasette/issues/647 | 1061226942 | IC_kwDOBm6k_c4_QQm- | 9599 | 2022-03-07T23:00:06Z | 2022-03-07T23:00:06Z | OWNER | This needs to take into account the changes made here: - #1439 In the new encoding scheme, `-` has a special meaning in a table name: https://docs.datasette.io/en/latest/internals.html#dash-encoding I think `~` is the right character to use to separate a database name from its hash. `~` should be a URL safe character according to Python's implementation of percent-encoding, see comment here: https://github.com/simonw/datasette/blob/c5791156d92615f25696ba93dae5bb2dcc192c98/datasette/utils/__init__.py#L1146-L1152 So the plugin could check for `dbname~hash` and react based on that. | {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} | 531755959 | |
https://github.com/simonw/datasette/issues/1651#issuecomment-1061169528 | https://api.github.com/repos/simonw/datasette/issues/1651 | 1061169528 | IC_kwDOBm6k_c4_QCl4 | 9599 | 2022-03-07T21:47:01Z | 2022-03-07T21:47:01Z | OWNER | Wow, this code is difficult to follow! Look at this bit inside the `get_format()` method: https://github.com/simonw/datasette/blob/bb499942c15c4e2cfa4b6afab8f8debe5948c009/datasette/views/base.py#L469-L478 That's modifying the arguments that were extracted from the path by the routing regular expressions to have `table` as ` dash-decoded value! So calling `.get_format()` has the side effect of decoding the table names for you. Nasty. | {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} | 1161584460 | |
https://github.com/simonw/datasette/issues/1654#issuecomment-1061181530 | https://api.github.com/repos/simonw/datasette/issues/1654 | 1061181530 | IC_kwDOBm6k_c4_QFha | 9599 | 2022-03-07T22:02:06Z | 2022-03-07T22:02:06Z | OWNER | https://docs.github.com/en/communities/setting-up-your-project-for-healthy-contributions/adding-a-code-of-conduct-to-your-project says this should be called `CODE_OF_CONDUCT.md` in order for GitHub to pick it up. | {"total_count": 0, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 0} | 1161969891 |