Skip to content

fix: giallozafferano ingredients #1981

Open
jknndy wants to merge 2 commits into
hhursev:mainfrom
jknndy:ingredients-giallozafferano
Open

fix: giallozafferano ingredients #1981
jknndy wants to merge 2 commits into
hhursev:mainfrom
jknndy:ingredients-giallozafferano

Conversation

@jknndy

@jknndy jknndy commented May 29, 2026

Copy link
Copy Markdown
Collaborator

See #1980 for discussion
@michaelplatt07 , let me know what you think of this implementation

@michaelplatt07

Copy link
Copy Markdown
Contributor

@jknndy Totally get why this implementation would be used in favor of the previous one I had opened. There were two things I noticed in the code though that give me slight pause. First, the units is a discrete list and would potentially break if another unit were used for some reason such as tsp or cup. Not sure if this is something that the scraper would care about for now but the test case doesn't cover something like Latte 1 tazza. Of course given this is an Italian site they would still likely not use that but I'm not familiar with the site and if there is anyone posting more Imperial friendly measurements it could present an issue if they were to use the scraper and assume it would reverse. The second thing that stands out is the expected output. Going from Latte intero 200 ml to 200ml Latte intero means you couldn't split on an ingredient and quantity nearly as easily downstream. I know for me and my uses in this scraper, I split the entire line so I have discrete pieces of data to feed or work on. Also, an example such as paninihappy.json seems to split like this 4 cups prepared macaroni and cheese, warmed for the expected result which means the results are inconsistent. Ultimately these are, I think, nitpicks I'm pointing out but they are just things I noticed. I went ahead and approved the PR in case these are not valid points as I don't want my input to be a blocker (especially given I just started contributing a couple days ago).

@michaelplatt07

Copy link
Copy Markdown
Contributor

@jknndy Hi there 👋 Just circling back here to see if there were any thoughts about the comments? I'll probably be throwing up some more PRs here this week for other sites I've run into just to continue to help provide support but I wanted to make sure this was closed out first so it can be an example for anyone if they run into this same situation moving forward.

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.

2 participants