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

ENH: Fail better on AssetFinder(nonexistent_path). #2000

Merged
merged 2 commits into from Oct 31, 2017

Conversation

Projects
None yet
4 participants
@ssanderson
Member

ssanderson commented Oct 27, 2017

Make the AssetFinder fail with a clearer error if you construct and
AssetFinder pointing to a file path that doesnt exist.

The current implementation fails with:

InvalidRequestError: Could not reflect: requested table(s) not available in sqlite:///<filepath>

which makes it look like the error was that the db was corrupted, when
the actual error was just that the file didn't even exist. This is made
worse by the fact that sqlite3 will actually create an empty file when
you call connect, which further propagates the appearance of having
previously had a bad file.

ENH: Fail better on AssetFinder(nonexistent_path).
Make the AssetFinder fail with a clearer error if you construct and
AssetFinder pointing to a file path that doesnt exist.

The current implementation fails with:

```
InvalidRequestError: Could not reflect: requested table(s) not available in sqlite:///<filepath>
```

which makes it look like the error was that the db was corrupted, when
the actual error was just that the file didn't even exist. This is made
worse by the fact that sqlite3 will actually create an empty file when
you call `connect`, which further propagates the appearance of having
previously had a bad file.
@coveralls

This comment has been minimized.

coveralls commented Oct 27, 2017

Coverage Status

Coverage increased (+0.01%) to 87.234% when pulling f4305fd on fail-usefully-on-asset-finder-of-nonexistent-path into 61c8869 on master.

@richafrank

Had some small suggestions, but LGTM. Thanks for this real diagnosis/debugging improvement!

class TestAssetFinderPreprocessors(WithTmpDir, ZiplineTestCase):
def test_asset_finder_doesnt_silently_create_useless_empty_files(self):
nonexistent_path = self.tmpdir.getpath('nothing_here')

This comment has been minimized.

@richafrank

richafrank Oct 31, 2017

Member

What do you think about using a unique name instead of 'nothing_here' to prevent any conflict between tests in this class?

This comment has been minimized.

@ssanderson

ssanderson Oct 31, 2017

Member

updated to prepend self.id()

This comment has been minimized.

@richafrank
@@ -358,7 +358,7 @@ class AssetDBWriter(object):
"""
DEFAULT_CHUNK_SIZE = SQLITE_MAX_VARIABLE_NUMBER
@preprocess(engine=coerce_string_to_eng)
@preprocess(engine=coerce_string_to_eng(require_exists=False))

This comment has been minimized.

@richafrank

richafrank Oct 31, 2017

Member

Yup, this looks like the one place we don't want to require it exists. Do we ever append, as in, would we want to require that it does NOT exist?

This comment has been minimized.

@ssanderson

ssanderson Oct 31, 2017

Member

I'm not sure about this offhand. @yankees714 do you know if we ever do inplace updates internally? My inclination is to say that even if we don't expect to update an asset db inplace, it's possible that another user might, so I don't feel compelled to disallow that by construction.

This comment has been minimized.

@yankees714

yankees714 Oct 31, 2017

Contributor

I'm not aware of any place where we append, but I agree that it probably makes sense to leave this flexible in case others want to.

@coveralls

This comment has been minimized.

coveralls commented Oct 31, 2017

Coverage Status

Coverage increased (+0.01%) to 87.234% when pulling 2350ff5 on fail-usefully-on-asset-finder-of-nonexistent-path into 61c8869 on master.

@ssanderson ssanderson merged commit 6114d90 into master Oct 31, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ssanderson ssanderson deleted the fail-usefully-on-asset-finder-of-nonexistent-path branch Oct 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment