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

Add giallo zafferano to scrapers, closes #28 #29

Merged
merged 3 commits into from
Oct 30, 2017

Conversation

marcolussetti
Copy link
Collaborator

@marcolussetti marcolussetti commented Oct 30, 2017

This Pull Request adds support for a new scraper for the website Giallo Zafferano, Italy's largest recipe site.

  1. To parse the instructions, I parsed embedded json with the standard library's json module. It seemed to me a cleaner option than trying to extract the various paragraphs of instructions that had no real markers (could do between tags, but it seemed not very clean and brittle). I can rewrite it to use pure beautifulsoup if preferred.
  2. I wrote tests for the code, but I have to admit I'm pretty new to it so I don't know if I went about it the right way. The test clear fine on my machine running against a saved html page obtained via:
curl -o recipe_scrapers/tests/test_data/giallozafferano.html http://ricette.giallozafferano.it/Bavarese-alle-fragole.html

I didn't add this file anywhere as I think that's in your hands ;-)

Please let me know if this is all right or if you would like anything changed ;-)

@hhursev hhursev force-pushed the master branch 2 times, most recently from 5ef7eb7 to 2cf7e9a Compare October 30, 2017 12:42
@hhursev
Copy link
Owner

hhursev commented Oct 30, 2017

This is perfect addition! Also I want to thank you for being extremely careful with the repo's code base. You even included the giallozafferano's link at the right place in the Readme.md file!

I’d like to make this PR a standard for forthcoming PRs. I tidied up the repo for you and made it possible to add tests. Can you please:

  • add tests for Giallo Zafferano
  • rebase the PR so to stay on top of the master branch (again)
  • last is needed as I love fast forward merges

P.P. Love what you did with the instructions! Leave it like that :)

Relying on JSON parsing for instruction as that seems less brittle than
filtering <p> tags between two generic divs.
@coveralls
Copy link

coveralls commented Oct 30, 2017

Coverage Status

Coverage increased (+0.2%) to 96.164% when pulling 208748a on marcolussetti:add-giallo-zafferano into deab530 on hhursev:master.

@marcolussetti
Copy link
Collaborator Author

marcolussetti commented Oct 30, 2017

Thank you for your kind words, it's a very easy repository to work with I think :)

I've added the test data so the tests from earlier should be working fine now and rebased the PR on top of it, hope that works :)

I'll try to remember to fast forward on future PRs if things get merged in between :)

@hhursev hhursev merged commit 39d94d1 into hhursev:master Oct 30, 2017
@klebinhopk
Copy link

Poderia me ajudar a rodar esse script do python por favor?
I From Brazil

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

4 participants