Skip to content
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

[WIP]Adding image scraping #42

Merged
merged 3 commits into from
Nov 18, 2018
Merged

Conversation

ba11b0y
Copy link
Contributor

@ba11b0y ba11b0y commented Oct 20, 2017

Adds to #18
I know it is violating the do not repeat yourself(DRY) principle, as I am not clear how should I include this addition.
@ashwini0529 If you could tell me if this is fine or should I improve upon the structure.

@ashwini0529
Copy link
Member

Included in requirements.txt?

@ba11b0y
Copy link
Contributor Author

ba11b0y commented Oct 22, 2017

@ashwini0529 I have updated the requirements as well as the usage. Please let me know if any corrections are needed.
Also I was thinking of defining a function outside the scope of the scrape function, to eliminate repetition of the code for scraping images.
Since that would be a major change to the code style, your thoughts?

@ashwini0529
Copy link
Member

Hey @invinciblycool I guess that would be better. 😄 Please go ahead with it! 🎉

@ba11b0y ba11b0y force-pushed the extend/webscraper branch 2 times, most recently from 14facf9 to 24f3e2a Compare October 23, 2017 07:37
@ba11b0y
Copy link
Contributor Author

ba11b0y commented Oct 23, 2017

@ashwini0529 I accidentally squashed the wrong number of commits. What should I do now? 😟
Closing the PR can be a solution, but is there any way out without closing the PR.

@ba11b0y
Copy link
Contributor Author

ba11b0y commented Oct 25, 2017

@ashwini0529 Then should I open a new PR, and close this.
Sorry again for the mess. 😟
I guess I need to go through squashing commits once again.

@ashwini0529
Copy link
Member

Haha Alright! You could fix it in this PR but if you think that opening a new PR would be easy as of now, go ahead! 👍

@ba11b0y
Copy link
Contributor Author

ba11b0y commented Nov 18, 2018

Done.
@ashwini0529 Intentionally kept the three commits instead of squashing them into one.

@ashwini0529 ashwini0529 merged commit ff30012 into pytorn:dev Nov 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants