Documentation and unit tests for urls.row() urls.row_blob() methods

[simonw]

datasette/datasette/app.py

Lines 1307 to 1313 in 5db7ae3

	def row(self, database, table, row_path):
	return "{}/{}".format(self.table(database, table), row_path)
	def row_blob(self, database, table, row_path, column):
	return self.table(database, table) + "/-/blob/{}/{}.blob".format(
	row_path, urllib.parse.quote_plus(column)
	)

[simonw]

The row_path part of these really isn't very user friendly, since you need to properly URL-encode the values. The safest way to do so is by calling this obscure, undocumented utility function:

datasette/datasette/utils/__init__.py

Lines 84 to 98 in f0bd2d0

	def path_from_row_pks(row, pks, use_rowid, quote=True):
	"""Generate an optionally URL-quoted unique identifier
	for a row from its primary keys."""
	if use_rowid:
	bits = [row["rowid"]]
	else:
	bits = [
	row[pk]["value"] if isinstance(row[pk], dict) else row[pk] for pk in pks
	]
	if quote:
	bits = [urllib.parse.quote_plus(str(bit)) for bit in bits]
	else:
	bits = [str(bit) for bit in bits]
	return ",".join(bits)

This feels like it should be improved before I turn it into a documented API.

(Note that this API deals with a row that is a potentially-nested dictionary - not with a sqlite3.Row object.)

[simonw]

Also related: row is now available to render_cell() hook as-of this issue:

• Make row available to render_cell plugin hook #1300

[simonw]

I'm considering changing these functions to accept the row object itself:

 def row(self, database, table, row): 
     ...
  
 def row_blob(self, database, table, row, column): 
     ... 

Just one catch: in order to generate the correct row_path we need to know the primary keys for that table. We can look those up based on having access to database and table, but doing so requires an await ... operation - and these functions are not async.

[simonw]

None of the potential solutions for that problem are particularly appealing:

• Make these URL generation methods async - should the other ones be async too for consistency?
• Have some kind of mechanism that calculates and caches the pks for each table somewhere which can then be accessed here without an async call
• Require pks is passed to these methods, having been looked up elsewhere. This is a bit gross but may end up being the best solution

[simonw]

If I do require pks to be passed here, maybe I could make those available to the render_cell() plugin hook to at least make this a bit more pleasant for plugin authors to use?

Current hook: https://docs.datasette.io/en/latest/plugin_hooks.html#render-cell-row-value-column-table-database-datasette

datasette/datasette/hookspecs.py

Lines 62 to 64 in 035dc5e

	@hookspec
	def render_cell(row, value, column, table, database, datasette):
	"""Customize rendering of HTML table cell values"""

The hook is called in two places in the codebase - when rendering a table (pks variable is already in scope here):

datasette/datasette/views/table.py

Lines 897 to 904 in 6373bb3

	for candidate in pm.hook.render_cell(
	row=row,
	value=value,
	column=column,
	table=table_name,
	database=database_name,
	datasette=datasette,
	):

And when rendering an arbitrary query:

datasette/datasette/views/database.py

Lines 377 to 384 in 6373bb3

	for candidate in pm.hook.render_cell(
	row=row,
	value=value,
	column=column,
	table=None,
	database=database,
	datasette=self.ds,
	):

Note that in that second one table is None (which is also called out in the documentation) - pks would be None here too.

[simonw]

I think the best way to do this is to change the method signatures to:

 def row(self, database, table, row, pks): 
     ...
  
 def row_blob(self, database, table, row, pks, column): 
     ... 

[simonw]

But do I need to pass the use_rowid boolean here as well, as used by def path_from_row_pks(row, pks, use_rowid, quote=True)? Or can I derive that from the fact that pks is an empty tuple?