Basic local FS functionality #3693
Conversation
* basic storage_adapter logic for dynamic loading
…xed, type hinting
| path: Path = Path(path) | ||
|
|
||
| # handle overwrite errors for files and directories | ||
| if path.exists() and (overwrite is False or path.is_dir() is True): |
There was a problem hiding this comment.
If directory already exists, that essentially is a no-op. Would you consider that not successful? I honestly don't know how that effects things in the code, but it just seemed odd to me
There was a problem hiding this comment.
Yeah it's a kind of funny interaction to model out. I do consider it a "fail" (hence it returns false), but one that may be expected under certain circumstances. For example, you can do things like to populate a directory with files (say our snowplow event cookie)
if SA.write('path/that/might/be/here', None):
SA.write('path/that/might/be/here/important_cookie_file.json`, TEMPLATE_STRING)
| path: Path = Path(path) | ||
|
|
||
| # ensure resource to be deleted exists | ||
| if not path.exists(): |
There was a problem hiding this comment.
Same question here, if it doesn't already exist, would you call it not successful?
| @@ -0,0 +1 @@ | |||
| Move the warehouse adapters to a subfolder of adapters (adapters/warehouse). | |||
There was a problem hiding this comment.
These are in plugins/ right now (and three of them are soon to leave this repo). Let's think about whether local_filesystem makes more sense under plugins/ or as part of core/.
Also... we'll need to come up with a new word for this, since none of warehouse, database, query engine, etc is quite perfect. To date, I've just been using "adapter" as a nice catch-all, but obviously we've got adapters for more things now :)
There was a problem hiding this comment.
@JerCo That makes sense. I'll do some folder juggling and see where we land. As far as naming goes.. I've been leaning towards "warehouse adapters" and "storage adapters", but I don't hold that opinion too strongly.
There was a problem hiding this comment.
lmk if you want to chat plugins! I think I have a good sense how that could look
Co-authored-by: leahwicz <[email protected]>
Co-authored-by: leahwicz <[email protected]>
| @staticmethod | ||
| def find() -> None: | ||
| pass |
There was a problem hiding this comment.
What is find responsible for? Is the same thing accomplished by info?
There was a problem hiding this comment.
No, it's a reminder to me that I need to incorporate the find_matching method from the dbt.client.system
| @@ -0,0 +1 @@ | |||
| Move the warehouse adapters to a subfolder of adapters (adapters/warehouse). | |||
There was a problem hiding this comment.
lmk if you want to chat plugins! I think I have a good sense how that could look
|
pulling myself off because it looks like you have plenty of reviews. Let me know if you need me for any part of this though. |
PR for first segment of local FS storage adapter.
Contains:
This breaks a few unit tests and 1 integration test. The unit test failures are utilizing MagicMocks which don't seem to play nice as file paths for some reason. The integration test failure is in partial parsing testing, needs further investigation