refactor!: Make default Apify storages use alias mechanism#606
refactor!: Make default Apify storages use alias mechanism#606
Conversation
github-actions
bot
added
t-tooling
Pijukatel
requested review from
janbuchar and
vdusek
and removed request for
vdusek
vdusek
left a comment
vdusek
left a comment
There was a problem hiding this comment.
One comment to the upgrading guide, I'll leave a proper review to @janbuchar.
docs/04_upgrading/upgrading_to_v3.md
Outdated
| ## Default storage ids in configuration changed to None | ||
| - `Configuration.default_key_value_store_id` changed from `'default'` to `None`. | ||
| - `Configuration.default_dataset_id` changed from `'default'` to `None`. | ||
| - `Configuration.default_request_queue_id` changed from `'default'` to `None`. | ||
|
|
||
| As a consequence of this change, using default storage without specifying its `id` in `Configuration` will use unnamed storage. |
There was a problem hiding this comment.
TBH, this doesn't explain much - even I'm not sure what it would mean for me or what the consequences are.
There was a problem hiding this comment.
I added a more detailed explanation.
| # 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.
But... this shouldn't happen thanks to the normalization step that you added, right?
There was a problem hiding this comment.
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.
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.
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.
Pijukatel
force-pushed
the
default-storage-apify-local
branch
from
bbf2069 to
6278302
Compare
janbuchar
left a comment
janbuchar
left a comment
There was a problem hiding this comment.
If you can fix that redundant ValueEror before merging this, I'd be much happier, but feel free to merge if that's too much trouble.
3 participants
Description
Issues
Closes: #599
Testing
Added unit tests