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

Added serving-size #61

Merged
merged 6 commits into from
Jun 12, 2019
Merged

Added serving-size #61

merged 6 commits into from
Jun 12, 2019

Conversation

Jwe0619
Copy link
Contributor

@Jwe0619 Jwe0619 commented Jun 9, 2019

Most of the given pages already have serving-size on the page. It gives a better overview when getting recipes when possible. Some pages, like All Recipes (BR), The vintage mixer and What's Gaby cooking do not have this possibility, no testcase created but will return 0.

The _utils.get_servings is not clean logic, it tries to see if the given element is for serving (defaults to serving) or items. Might not be the best practice.

Added get_servings to _utils.
…t possible for IDE or single unittest run).

Added serving-test to all except All recipes BR, The vintage mixer and What's gaby cooking (do not have servings in test-cases), will return 0.
@hhursev
Copy link
Owner

hhursev commented Jun 10, 2019

Woah man 🍻 This is a really nice job! The code looks spot on.

The only thing that I find strange is the one you've been upfront about - get_servings returning either int or str.

If I understand correct, serving is more like a portion you eat on a single sitting and items is used when we talk about cakes, sandwiches etc.

I believe the best approach would be to rename the method to yields and make it return a str. The result may be "4 servings", "5 items", "8 portions" etc. The method defaults should default to empty str.

Let me know if this makes sense and you agree with the logic.

I really appreciate the input. The code is 🥇 👍

yields-method will now return a string of "x item(s)" or "x serving(s)", will default toward servings when found a number, will default toward an empty string if it defaults of missing content.
@Jwe0619
Copy link
Contributor Author

Jwe0619 commented Jun 11, 2019

That's great input, got a bit blind by "serving".
I simplified it, since I want it to be easy to get the serving size when it exists. So yields will return "x serving(s)" if it's of "portions" and "item(s)".

This is not 100% correct though, like jamie olivers test-case it says "serving(s)" when it should be "item(s)", but this is because the page literally says "Serves" for the items. So it is not without an error rate, but it is redundant since some common sense can be used (if it is a muffins recipe and it has 20 servings, the human conclusion should be that we are talking about number of muffins).

So the debate is rather if it should have "item(s)" or only "serving(s)", but I feel it is nice to have items-category when possible. Let me know what you think!

@hhursev
Copy link
Owner

hhursev commented Jun 12, 2019

I'd expect this package to parse what is on the site without playing it extra clever. Even for the oliver's site I'd make it dumb and just return "5 serve(s)".

That being said I'd expect the yields method to return:

2 item(s)
1 serve(s)
5 serving(s)

And whatever the site is using for that recipe. That way people using the package can determine the categories by split(' ')[1] and unite them however they find fit. I can see how different scenarios would require different grouping.

lmk your thoughts on this and feel free to add the new method in the README.md file too! I'd like to merge this :)

@Jwe0619
Copy link
Contributor Author

Jwe0619 commented Jun 12, 2019

To be fair, it is that dumb, I matched "dumbness level" with the total_time. It takes the text and format it to a text that is easy to split as you said. The reason why I formatted it is because it's arbitrary how the text is represented, almost no page presents it text-wise the same. I just make sure that it is <#> <helptext>, and it will be serving(s) in 90% of the cases. The reason why some returns are item(s) as the helptext is mostly an hint that we are probably not talking about servings without any guarantees. If people want to use that or not is outside the scoop of the package. So yeah, it is dumb alright.

But if you really want it dumb I'll just have it return the integer without any help-string attached.

@hhursev hhursev merged commit 01ee1f5 into hhursev:master Jun 12, 2019
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