-
Notifications
You must be signed in to change notification settings - Fork 1
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
Fixed filename too long and empty image file errors, removed redundant example notebooks #29
Conversation
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.
Just some small questions regarding how it fixes the issues. Otherwise, nice work in cleaning up the notebooks.
artscraper/googleart.py
Outdated
return Path(self.output_dir, paint_id) | ||
# Prevent problems with too-long file/directory names | ||
path_str = str(Path(self.output_dir, paint_id)) | ||
if len(path_str)>=256: |
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.
If I understand correctly, this gives a directory, right? What is the problem exactly? The /x/y/z, where the whole thing is too long, or only if x or x or z are too long?
What if the output_dir path is already too long?
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.
I fixed this using SHA-1 hash.
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.
I think you might want to clear the outputs, just in case.
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.
I removed the outputs that I thought needed removing.
Hi @qubixes : I have made the changes you requested. Does it look okay now? |
Hi @qubixes: I have now made the change you requested yesterday (set filename truncation length to 240). |
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.
I approve the changes!
Hi @qubixes , @jgarciab :
I have fixed the issues mentioned in the title. Could you please review my code and accept the pull request if everything looks ok?
Fixes #28 .
Thanks,
Modhurita