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

Hebrew long-form spelling, gender, ordinals, fractions, maxval=1e66, construct forms, etc #490

Merged
merged 26 commits into from
Jan 22, 2023

Conversation

eyaler
Copy link
Contributor

@eyaler eyaler commented Dec 2, 2022

Fixes Eyal Gruss

Changes proposed in this pull request:

  • Hebrew:
    ** change spelling from biblical and short-form (ktiv haser) to modern long-form (ktiv male)
    ** male/female genders (default to female for cardinal and to male for ordinal)
    ** ordinals (with optional definite and plural forms)
    ** decimal fraction and negatives
    ** fix currency name (NIS->ILS), add currency genders, pluralize סנט, add optional singular forms for plural when allowed.
    ** fix word order for singular form with currency
    ** fix "two" to always use construct form with currency
    ** construct forms (סמיכות)
    ** max val = 1e66 (up from 10,000)
    ** simplify tables for hundreds and thousands (DRY)
    ** add tests

  • Global:
    ** fix potential unintended double-space after negword
    ** fix new lines in test CLI

Status

  • READY
  • HOLD
  • WIP (Work-In-Progress)

How to verify this change

for spelling search the word here: https://hebrew-academy.org.il/ and observe the form labeled "ללא ניקוד" (=without diacritics)

Additional notes

about short vs long form: https://hebrew-academy.org.il/topic/hahlatot/missingvocalizationspelling/

note that for this commit i chose to override the short-form. a future feature could bring back (non-biblical) short-form as a non-default secondary option, preferably with diacritics. the biblical form (e.g. שלש), could be yet another option.

about decimal fractions: https://hebrew-academy.org.il/2019/12/03/%D7%A2%D7%9C-%D7%94%D7%91%D7%A2%D7%AA-%D7%94%D7%9E%D7%A1%D7%A4%D7%A8-%D7%94%D7%9E%D7%A2%D7%95%D7%A8%D7%91/

Note that there are actually 4 allowed forms. i chose the one the seems more relevant to the convention of digit by digit readout after the decimal point.

For large numbers millions, etc. there are various allowed variations, i chose the simplest one (singular non-construct)

Missing features: add comma separators in long numbers, allow hyphen (or hebrew hyphen) for numbers in construct form

If applicable, explain the rationale behind your change.
This brings the Hebrew support to usable level for modern use.

@eyaler eyaler changed the title Hebrew long-form spelling, gender, ordinals, fractions, etc Hebrew long-form spelling, gender, ordinals, fractions, maxval=1e15, construct forms, etc Dec 3, 2022
@eyaler eyaler changed the title Hebrew long-form spelling, gender, ordinals, fractions, maxval=1e15, construct forms, etc Hebrew long-form spelling, gender, ordinals, fractions, maxval=1e66, construct forms, etc Dec 3, 2022
This was referenced Dec 4, 2022
@eyaler
Copy link
Contributor Author

eyaler commented Jan 5, 2023

sorry not sure if there is anything i need to do?
@mrodriguezg1991

@mrodriguezg1991
Copy link
Contributor

@eyaler The issue is related to the Flake8 max character in a line, you can check the test report to see which lines need adjusting, thanks for your contribution

@eyaler
Copy link
Contributor Author

eyaler commented Jan 6, 2023

@mrodriguezg1991 fixed. flake8 + tox pass ok on my side

@coveralls
Copy link

coveralls commented Jan 6, 2023

Coverage Status

Coverage: 97.17% (+0.2%) from 96.979% when pulling e272daa on eyaler:master into f510792 on savoirfairelinux:master.

@mrodriguezg1991
Copy link
Contributor

@eyaler can you add some tests?, the coverage decreased with this PR, thanks

@eyaler
Copy link
Contributor Author

eyaler commented Jan 13, 2023

@mrodriguezg1991 i actually added a lot of tests! could you point me out where the coverage is lacking?

@mrodriguezg1991
Copy link
Contributor

mrodriguezg1991 commented Jan 18, 2023

@mrodriguezg1991 i actually added a lot of tests! could you point me out where the coverage is lacking?
@eyaler
Can you check this file? https://coveralls.io/jobs/112440482/source_files/1353274836 , you can see the missing lines by coverage, thanks

@eyaler
Copy link
Contributor Author

eyaler commented Jan 20, 2023

@mrodriguezg1991 thanks for your guidance!

  1. i have tried to address all coverage issues which included adding tests and making the code more robust
  2. is there an easy way i can check the coverage after my changes?
  3. i fixed a small issue in base tests, where the test was failing silently. the test was intended to check the type of the variable, but it was capturing a typeerror due to wrong signature because a class method was being called as a static method. I wonder how we can make sure that this does not happen in the future.

@mrodriguezg1991 mrodriguezg1991 merged commit 3ef32f0 into savoirfairelinux:master Jan 22, 2023
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