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

Review suggestions for #613 #614

Closed
wants to merge 4 commits into from
Closed

Conversation

jayaddison
Copy link
Collaborator

  • Add some test coverage
  • ...partly to explain omitting elements with attribute class (I was trying to figure this out during code review)
  • Code style fixups

This made me think that it feels less straightforward than it should be to add multiple test cases per scraper.

cc @vabene1111 - feel free to merge into your pull request, and/or I can do if you're good with these suggested changes.

vabene1111 and others added 4 commits September 30, 2022 18:43
ingredient tables contained empty spacer or header rows, ignored those when importing to prevent exception
@jayaddison jayaddison mentioned this pull request Oct 1, 2022
@vabene1111
Copy link
Collaborator

This made me think that it feels less straightforward than it should be to add multiple test cases per scraper.

to be honest I just ran out of time and forgot about it afterwards, there are plenty of examples of having multiple test cases. Its just a bit renaming.

I like the changes, feel free to merge them and just close my PR as yours contains my changes already. If want to do me a favor add a label called hacktoberfest-accepted to my PR as I am participating in hacktoberfest and it would be great if it counts :)
Btw. hacktoberfest is really cool, I have been participating the last 6 years or so and its always a welcome occasion to work on something completely different and they send out a really nice shirt https://hacktoberfest.com/

@jayaddison
Copy link
Collaborator Author

Changes merged into #613.

@jayaddison jayaddison closed this Oct 1, 2022
@jayaddison jayaddison deleted the pr-613/review-suggestions branch October 1, 2022 16:35
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