Skip to content

Improvements, refactoring and German language - #4

Closed
benjackwhite wants to merge 7 commits into
suprmat95:masterfrom
benjackwhite:main
Closed

Improvements, refactoring and German language#4
benjackwhite wants to merge 7 commits into
suprmat95:masterfrom
benjackwhite:main

Conversation

@benjackwhite

Copy link
Copy Markdown

I'm going to have another attempt at merging this in here as I think I've made some solid improvements to the core of the codebase and I have a vested interest in the project (I'm using my fork actively in my app https://mealyo.app).

Major changes

  • Splitting out of languages to separate files and adding of German language
  • Quality of life improvements like automate-actions, simplified linting tools
  • Fixing of a lot of failing tests
  • Typing improvements
  • This also has a slightly nicer regex solution for units which solves Escape '.' on eng singular units. #3 without needing to comment out . characters

Other Suggestions

  1. I removed the q.b. code. This is Italian specific but is in the shared code which isn't great.
  • Suggestion - keep it removed. This is quite a specific logic and imo should be handled by the calling code and not by the library
  1. Suggestion multiLineParse should be removed. This is again quite specific and depends on the caller (especially the hard coded emojis). This lib parses ingredients from a string, I think making it also have logic for splitting lines based on something that is clearly
  2. Make me a maintainer 😀
  • I think this repo has the potential to become the canonical ingredient-parsing repo for JS (there are a bunch of forks currently - almost one per language). With an extra maintainer I can help drive things forward (my languages are English and German) as well as be a sounding board for these more questionable additions (like the two I mentioned above).

scolobey and others added 7 commits June 11, 2022 07:36
At least for English, the units list contains both caps and lowercase options. This becomes important because 't' designates teaspoon and 'T' designates Tablespoon.

In addition to accurately identifying these edge cases 't' and 'T', the matched text is returned and used as the 'originalUnit' later to remove the unit from the ingredient string. This was causing the following commented English test to fail.
"25 lb beef stew chunks (or buy a roast and chop into small cubes)"

Because 'lb' was matching for both 'lb' and 'Lb' in the units list, and 'Lb' comes second, the final match being returned was for 'Lb' so when the unit is later removed via a replace() it tries to remove 'Lb' and finds nothing, so the ingredient text ends up being 'lb beef stew chunks' instead of 'beef stew chunks'
Remove 'case insensitive' designator on singular unit match regex.
* Added Prettier
* Added missing dev dependencies
* Formatted all files
* Split out test files
* Fixed failing tests related to english language
* Fixed combination and prettyprint logic
* Added CI tests
* Removed unnecessary capitalised duplicates
* Removed logs
* Refactored nested ifs
* Corrected typescript error
* Removed prettier config lines
* Type fixes
* Accounted for words like "butter" when looking for "to taste" regex (i.e. "tt")
* Refactored into multiple language files
* Added German Language
* Updated README and removed duplicate locks
* Added support for multiple joiners and numbers like 1,200.30
* Added support for commas instead of periods for decimals
* Modified regex detector to allow for Umlaut characters that are word breaking
* Fixed unicode fractions
* Added "Bund" german term
* Corrected order of English items
* Fixes some German translations and adds KL
* Added linting workflow

Co-authored-by: Sebastian Weggesser <sebastian.weggesser@googlemail.com>
# Conflicts:
#	package-lock.json
#	src/index.ts
#	src/numbers.ts
#	src/units.ts
#	test/index.test.ts
@benjackwhite

Copy link
Copy Markdown
Author

@suprmat95 if you enable Issues on the project I can happily create an issue to discuss adding me as a Maintainer separate to this PR.

If you are not interested in outside contributions that's totally fine, just let me know and I can publish my own package (not so ideal) to npm and go from there. @scolobey seems to have some interest in this repo as well so can weigh in with their thoughts

@suprmat95

suprmat95 commented Jun 13, 2022

Copy link
Copy Markdown
Owner

Hi, @benjackwhite and @scolobey your contributions are welcome :)

As @benjackwhite said, I use this parser for an Italian important project, so it's really important that all the Italian tests I already wrote continue to work. I will continue to work mainly on the Italian language because my project still supports only Italian.

About your suggestions:

  1. I can't remove q.b. It's the most used quantity in Italian recipes xD. It's the same for t.t. in English
  2. Many people have the recipes saved on the notes of their smartphone and they copy and paste ingredients on my application. I think that many other applications or projects have the same need and for this, I integrate this feature directly into this library. There are two separate functions to handle one or more strings.
  3. Yes If you agree not to break what I have done for the Italian language!

I believe in open source and I want to accept your contributions. Together we can become the canonical ingredient-parsing repo for JS :)

@scolobey

scolobey commented Jun 13, 2022

Copy link
Copy Markdown

The goal as I see it is a workable base incorporating most of the @benjackwhite changes.

This is the @benjackwhite test for no unit/no quantity.

it("doesn't explode when no unit and no quantity provided", () => {
    expect(parse('zucchero a velo', 'ita')).to.deep.equal({
      unit: null,
      unitPlural: null,
      symbol: null,
      ingredient: 'zucchero a velo',
      quantity: 0,
      minQty: 0,
      maxQty: 0,
    });
  });

If I understand correctly @suprmat95 needs something more look like this...

it("doesn't explode when no unit and no quantity provided", () => {
    expect(parse('zucchero a velo', 'ita')).to.deep.equal({
      unit: 'q.b',
      unitPlural: 'q.b',
      symbol: null,
      ingredient: 'zucchero a velo',
      quantity: 0,
      minQty: 0,
      maxQty: 0,
    });
  });

Is that correct @suprmat95 ?

If so, I think we just need a quick solution for getting the amended test to pass without causing problems for eng or deu languages. Then we can explore improved solutions in future PR's?

Here is one possible solution where all tests should be passing, including the amended (no unit/no quantity) test as described. #5

@benjackwhite

Copy link
Copy Markdown
Author

I believe in open source and I want to accept your contributions. Together we can become the canonical ingredient-parsing repo for JS :)

I think so too! But for it to be that we need to make sure no more Italian-only changes make it in to the repo like the q.b. change as then it only is useful for you 😅 (and other Italian speakers).

@scolobey's changes make sense to me 👍

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.

3 participants