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

Change how reserved columns work to have an underscore prefix #14

Closed
simonw opened this issue Nov 13, 2021 · 5 comments
Closed

Change how reserved columns work to have an underscore prefix #14

simonw opened this issue Nov 13, 2021 · 5 comments
Labels
enhancement New feature or request

Comments

@simonw
Copy link
Owner

simonw commented Nov 13, 2021

Right now I've implemented it such that id and commit and version and item are reserved columns, and any user-provided data with those columns gets renamed to commit_ and id_ and so-on - see #8.

I've changed my mind. I think this tool's columns should all have a _ prefix instead (like semi-private class properties in Python).

I'm even going to do that for the _id column, mainly so I don't have to explain a special case for id.

@simonw simonw added the enhancement New feature or request label Nov 13, 2021
@simonw
Copy link
Owner Author

simonw commented Nov 13, 2021

I'm also going to change how "banned" columns work. Currently if your data has a item_ column the tool raises an error. The new reserved column will be _item but my rule will be that if your column is reserved I'll add a trailing _ to it - and if you also have a _item_ column I'll rename that to _item__ and so-on.

Current implementation:

def fix_reserved_columns(item, allow_id=False, allow_banned=False):
reserved = {"item", "version", "commit", "rowid"}
banned = {"id_", "item_", "version_", "commit_"}
if not allow_id:
reserved.add("id")
if not allow_banned and any(key in banned for key in item):
raise click.ClickException(
"Column {} is one of these banned columns: {}\n{}".format(
sorted([key for key in item if key in banned]),
sorted(banned),
json.dumps(item, indent=4, default=str),
)
)

@simonw
Copy link
Owner Author

simonw commented Nov 13, 2021

I'm going to split out some of this logic into a utils.py module, and write some separate tests for it.

@simonw
Copy link
Owner Author

simonw commented Nov 14, 2021

Well this was a mistake... Datasette treats columns that start with an underscore as querystring parameters it should ignore!

bchydro__item_versions__99_918_rows

Switching to underscore suffixes instead.

@simonw
Copy link
Owner Author

simonw commented Nov 14, 2021

... I do like underscore prefixes though. I'm tempted to fix the bug I filed in Datasette instead and keep this:

@simonw
Copy link
Owner Author

simonw commented Nov 14, 2021

I released Datasette 0.59.2 with a fix for this, so I'm free to use _id and friends here.

@simonw simonw closed this as completed in 70e8955 Nov 14, 2021
simonw added a commit that referenced this issue Nov 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant