-
Notifications
You must be signed in to change notification settings - Fork 3
support transforming google images #419
support transforming google images #419
Conversation
64d325c
to
1652902
Compare
@@ -56,5 +56,5 @@ def run(origin_path: str, destination_path: str, api: str, arg_files: str) -> No | |||
version_prefix = api + "/" | |||
for result in results: | |||
result.filename = destination_path + "/" + version_prefix + result.filename | |||
print("put content to filesystem - filename: " + result.filename) | |||
# print("put content to filesystem - filename: " + result.filename) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove this line? 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise this looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I will remove it 😄
import os | ||
|
||
|
||
def test_transformer_google(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a docstring to your tests as well as breaking test cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I can add a docstring. How would you model a breaking test? The only thing that I have in mind is, that the transformer would fail if we provide an invalid input file. Does this make sense to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that makes total sense to me. I would add
- Files with a broken structure (test case: the retriever created a malformed file)
- Files cannot be downloaded (test case: S3 bucket is down / cannot be reached)
I want to make sure that we can catch those failures should they occur.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Files with a broken structure (test case: the retriever created a malformed file)
Sure, I will create an issue 👍 #421
Files cannot be downloaded (test case: S3 bucket is down / cannot be reached)
I want to make sure that we can catch those failures should they occur.
Is that then a test of the s3cmd tool? 🤔
Since the transformer only uses files from the filesystem. We removed the s3 bucket connection (implementation), remember? 🥴
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. Strike the download! We'll cover empty inputs with the rest of the tests in the follow up task!
Signed-off-by: Janine Olear <pninak@web.de>
for more information, see https://pre-commit.ci
Signed-off-by: Janine Olear <pninak@web.de>
1652902
to
f150995
Compare
for more information, see https://pre-commit.ci
closes #405