-
Notifications
You must be signed in to change notification settings - Fork 1
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
getQuantity not working correctly for ingredients without a quantity #171
Comments
Hello there! Seems like a bug related to how the word "and" is used to separate quantities (e.g. 1 and 1/2). I should be able to have a look this weekend. As for the demo site, good question. I don't recall what version it is, but I like the idea to show the package version there somewhere. |
Thanks so much for your reply! That sounds great! I actually did eventually get it to work for that ingredient and it doesn't seem to conflict with anything else. It was setting the index to 6 which was causing it to miss "Pinch salt". I just added a check at the end of getQuantity that checks if the result is just the word "and", then reset the index and don't return anything.
Hopefully that helps, I think you would know a lot better than me if this would cause other issues. Btw you're a hero for making and maintaining this repo! |
Hey @Bash4195. Thanks! I use this library a lot on my recipe book app. I'm glad other people can use it too. I spent some quality time with the code but didn't get a great solution. I was able to fix the "and" short-circuiting the getQuantity result. However, the ($0.05) is now been identified as a quantity. Any other alternative I tried breaks a ton of unit tests. The screenshot below is from the unit test complaining about unexpected quantity. You can see my tiny changes in the commit 15a43e1. How much do you need that 0.05 at the end of the ingredient string? If we remove that part of the ingredient string the change above works as expected. |
I forgot to mention. The demo site uses one of the very first versions of the library that I made available. I never noticed how outdated it was. Currently, it uses version 1.1.0-beta.2. I will update it shortly. |
I got this working, but it was more expensive than I expected. PR #172 contains:
|
After testing for a while longer, the cost of the dictionary lookup wasn't as high as initially anticipated. I just closed the PR and will release a new package version and demo site with the changes. Please let me know if this fixes your problem. |
Hey there sorry for the late response, I wasn't feeling well this week. Thanks so much for these changes! I've tested it on my end and it's definitely looking better. Your solution is a lot cleaner than mine too. I did find one other scenario where it breaks down. With an ingredient like "Whole chicken, ($1.00)" it ends up thinking the quantity is 1 and doesn't get the ingredient after that. I was able to fix it by removing the parenthesis stuff before the call to getQuantity. Quick solution copied from elsewhere in the file:
That was the last major issue I found. |
Hey @Bash4195, I'm glad the previous solution worked for some scenarios. It didn't work for "Whole Chicken, ($1.00)" because there is no UOM to short-circuit the get number so it will go to the end and find the number within parenthesis. I don't know if there is a solution that won't break the other scenarios. Do you mind creating a new issue for this problem? I will try to solve it but for what I checked tonight, no luck... |
This example "Pinch salt and pepper ($0.05)" from BudgetBytes results in the ingredient being set to "pepper" and the quantity to " and".
Also could you please include a version number on the demo page? I'm not sure what version it's using but when entering this on that page, it seems to get confused when the parenthesis are included. But when I remove the parenthesis, it works perfectly. I've customized this package on my end to remove the parenthesis in the getQuantity function, but I don't get the same result as the demo, just the result as described previously. It looks like whatever version this demo uses has the solution to my problem but I'm not able to see what that is.
The text was updated successfully, but these errors were encountered: