-
Notifications
You must be signed in to change notification settings - Fork 15
refactor!: Make default Apify storages use alias mechanism #606
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -124,8 +124,10 @@ async def open( | |
) | ||
apify_datasets_client = apify_client_async.datasets() | ||
|
||
# Normalize 'default' alias to None | ||
alias = None if alias == 'default' else alias | ||
# Normalize unnamed default storage in cases where not defined in `configuration.default_dataset_id` to unnamed | ||
# storage aliased as `__default__` | ||
if not any([alias, name, id, configuration.default_dataset_id]): | ||
alias = '__default__' | ||
|
||
if alias: | ||
# Check if there is pre-existing alias mapping in the default KVS. | ||
|
@@ -150,6 +152,11 @@ async def open( | |
# If none are provided, try to get the default storage ID from environment variables. | ||
elif id is None: | ||
id = configuration.default_dataset_id | ||
if not id: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But... this shouldn't happen thanks to the normalization step that you added, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It will not happen, but mypy is not able to figure it out. Probably the code could be refactored to make it clearer to mypy, but I would leave it after the release, as it will be non-breaking pure refactoring, similar to #595 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't we just use https://docs.python.org/3/library/typing.html#typing.assert_never? I don't believe that we'll ever come back to change this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is too much runtime checking. The mypy has no clue about max 1 kwargs of three runtime checks and so on. Let's not bend the code too much for the mypy's sake. Anyway any such refactoring is non-breaking, so no need to rush it in before the release. |
||
raise ValueError( | ||
'Dataset "id", "name", or "alias" must be specified, ' | ||
'or a default dataset ID must be set in the configuration.' | ||
) | ||
|
||
# Now create the client for the determined ID | ||
apify_dataset_client = apify_client_async.dataset(dataset_id=id) | ||
|
Uh oh!
There was an error while loading. Please reload this page.