Skip to content
This repository has been archived by the owner on Oct 5, 2022. It is now read-only.

requested changes from commit after issue #5 #8

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

loopyd
Copy link

@loopyd loopyd commented Mar 10, 2018

from issue #5:

  • includes the hash argument as an option with affecting the url argument's functionality (you can still use the program as before, but now with the addition of --hash yes if you wish
  • includes the error counter you voiced you liked, as a property of download_image
  • slight output cleanup - errors where being printed twice in your version (unnecessary), exception is now passed directly once to error() and printed optimally
  • no extra config file needed, this one directly uses properties of download_image

includes the hash argument as an option with affecting the url argument's functionality
includes the error counter
slight output cleanup - errors where being printed twice (unessecary), exception is now passed directly once to error() and printed optimally
@loopyd
Copy link
Author

loopyd commented Mar 11, 2018

FYI, your change to use io.open instead of open produces the same error i was intending to fix in python 2.7 with my structural workarounds. be advised you will have to patch this. i left it intact to show you why this occours. (check travis)

The correct method for 2.7, is:

        with io.open('dataset/logs/google/source.html', 'w', encoding='utf-8') as f:

The python 3 syntax works and should be left as it is.

Sorry there where two here, forgot the encoding specifier.

@rushilsrivastava
Copy link
Owner

rushilsrivastava commented Mar 11, 2018

Will start the review when build checks with Travis CI.

@rushilsrivastava
Copy link
Owner

rushilsrivastava commented Mar 11, 2018

Opening the file with utf-8 encoding should be enough, correct? Could you give me an example to test with?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants