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

Handle spaces in DB names #590

Merged
merged 2 commits into from Nov 4, 2019
Merged

Conversation

@rixx
Copy link
Contributor

rixx commented Oct 11, 2019

Closes #503

@simonw

This comment has been minimized.

Copy link
Owner

simonw commented Oct 14, 2019

We need some unit tests for this.

I'd be fine with us renaming extra_database.db to extra database.py in these existing fixtures:

datasette/tests/fixtures.py

Lines 174 to 185 in af2e6a5

@pytest.fixture(scope="session")
def app_client_two_attached_databases():
yield from make_app_client(
extra_databases={"extra_database.db": EXTRA_DATABASE_SQL}
)
@pytest.fixture(scope="session")
def app_client_two_attached_databases_one_immutable():
yield from make_app_client(
is_immutable=True, extra_databases={"extra_database.db": EXTRA_DATABASE_SQL}
)

Then we could add an extra test_database_name_with_spaces(...) test in test_api.py which uses parametrize to exercise a bunch of URLs and check they all return a 200 - something a bit like this but covering a range of URL paths (one for database, table, row and table with filters):

datasette/tests/test_api.py

Lines 746 to 765 in 90d4f49

@pytest.mark.parametrize(
"path,expected_rows,expected_pages",
[
("/fixtures/no_primary_key.json", 201, 5),
("/fixtures/paginated_view.json", 201, 5),
("/fixtures/no_primary_key.json?_size=25", 201, 9),
("/fixtures/paginated_view.json?_size=25", 201, 9),
("/fixtures/paginated_view.json?_size=max", 201, 3),
("/fixtures/123_starts_with_digits.json", 0, 1),
# Ensure faceting doesn't break pagination:
("/fixtures/compound_three_primary_keys.json?_facet=pk1", 1001, 21),
# Paginating while sorted by an expanded foreign key should work
(
"/fixtures/roadside_attraction_characteristics.json?_size=2&_sort=attraction_id&_labels=on",
5,
3,
),
],
)
def test_paginate_tables_and_views(app_client, path, expected_rows, expected_pages):

@rixx

This comment has been minimized.

Copy link
Contributor Author

rixx commented Oct 14, 2019

Ah, thank you – I saw the need for unit tests but wasn't sure what the best way to add one would be.

@rixx rixx force-pushed the rixx:issue-503-names-with-spaces branch from 9ba6d67 to 34ee816 Oct 14, 2019
Closes #503
@rixx rixx force-pushed the rixx:issue-503-names-with-spaces branch from 34ee816 to 9d54ca9 Oct 14, 2019
@rixx

This comment has been minimized.

Copy link
Contributor Author

rixx commented Oct 14, 2019

Added tests.

@simonw simonw merged commit 931bfc6 into simonw:master Nov 4, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
simonw added a commit that referenced this pull request Nov 11, 2019
simonw added a commit that referenced this pull request Nov 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.