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

Support for formatting Slovenian #136

Merged
merged 1 commit into from
Nov 28, 2020

Conversation

filips123
Copy link
Contributor

@filips123 filips123 commented Oct 20, 2020

This adds support for formatting Slovenian numbers, dates, times and durations. It seems to work on all examples and tests I tested.

But there is one small problem that nice_duration only supports basic plural form while Slovenian uses many plural forms. I was able to fix this in other functions, but nice_duration relies on plural files (day.word and days.word, etc.), so there will be some wrong plural forms in some cases. However, it should still be understandable for people. I might check how I could fix this in future separate PR (it might require rewriting nice_duration plural support to something that works with more languages).

I will probably also create separate parsing PR in the future.

I have already signed CLA but it is may not be approved yet.

@devs-mycroft devs-mycroft added the CLA: Needed Need signed CLA from https://mycroft.ai/cla label Oct 20, 2020
@ChanceNCounter
Copy link
Contributor

nice_duration is one of the trickier functions. For a first pass, I absolutely wouldn't worry about complex cases; the LF team isn't likely to call a language "fully supported" for quite a while after initial work has been done.

After all, the upcoming release is v0.2.3, and a pending refactor will make it 0.3.0 👅 so "slow and steady" is still the name of the game in most languages.

At any rate, LF's a small team, so code review might take a little while. But I can see that you're localizing the right things, in what looks like the right way, and you've had success using the functions, so I'm gonna seize the initiative and give you your Hacktoberfest credit. I imagine @krisgesling will be along to speak to the CLA, and anything else that might need saying from Mycroft-the-company's perspective (I'm not an employee, just a collaborator on this package.)

@ChanceNCounter ChanceNCounter added the hacktoberfest-accepted Credit a PR with Hacktoberfest goodness, without merging. label Oct 20, 2020
@krisgesling krisgesling added CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) and removed CLA: Needed Need signed CLA from https://mycroft.ai/cla labels Oct 29, 2020
@krisgesling
Copy link
Contributor

CLA received, and you've been added to our list of excellent humans, thanks!

@MycroftAI MycroftAI deleted a comment from devs-mycroft Oct 29, 2020
@JarbasAl JarbasAl added the sl relates to slovenian language label Nov 2, 2020
@ChanceNCounter
Copy link
Contributor

Alright. I've rebased this for compatibility with LF 0.3+

@filips123 if you wouldn't mind checking out that branch (ChanceNCounter/format-slovenian) and ensuring that everything works, you can --force push it on top of this PR branch (I didn't wanna force-push someone else's repo)

It might need some touching up to conform with @JarbasAl's unified formatting and locations, I haven't looked yet. I've just done the basic rebase and integration.

@filips123 filips123 force-pushed the format-slovenian branch 2 times, most recently from 0bcde4a to 6813b4b Compare November 26, 2020 13:08
@filips123
Copy link
Contributor Author

Thanks! I tested it and everything seems to work correctly. I also have an idea how to improve plural support in nice_duration and I will probably create separate PR for this next month (because it will also be useful for other languages).

@ChanceNCounter
Copy link
Contributor

@krisgesling @JarbasAl Now having worked on this PR a little, I'd appreciate another look before it gets merged. Also, it might not be conformant with Jarbas' new layout. The localizer's finding everything, though, so either Jarbas is magic, everything's in the right place, or both.

Copy link
Collaborator

@JarbasAl JarbasAl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some nitpicks, but overall looks good

lingua_franca/lang/parse_sl.py is an empty file, can you at least add a comment, TODO implement parsing function

lingua_franca/res/text/sl-si/normalize.json Outdated Show resolved Hide resolved
test/test_format_sl.py Outdated Show resolved Hide resolved
test/test_format_sl.py Outdated Show resolved Hide resolved
lingua_franca/lang/common_data_sl.py Show resolved Hide resolved
@ChanceNCounter
Copy link
Contributor

ChanceNCounter commented Nov 26, 2020

It can probably be addressed in another PR, but, at the moment, parse.py has to exist.

If it doesn't, this happens:

>>> from lingua_franca import parse
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/Chance/lingua-franca/lingua_franca/parse.py", line 33, in <module>
    populate_localized_function_dict("parse", langs=get_active_langs())
  File "/Users/Chance/lingua-franca/lingua_franca/internal.py", line 643, in populate_localized_function_dict
    mod = import_module(".lang." + lf_module + "_" + primary_lang_code,
  File "/usr/local/Cellar/[email protected]/3.8.5/Frameworks/Python.framework/Versions/3.8/lib/python3.8/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
ModuleNotFoundError: No module named 'lingua_franca.lang.parse_sl'

whereas, with the empty file,

>>> lingua_franca.set_default_lang("sl")
>>> from lingua_franca import parse
>>> parse.extract_datetime("one")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/Chance/lingua-franca/lingua_franca/internal.py", line 592, in call_localized_function
    return _call_localized_function(func, *args, **kwargs)
  File "/Users/Chance/lingua-franca/lingua_franca/internal.py", line 550, in _call_localized_function
    raise loc_signature
lingua_franca.internal.FunctionNotLocalizedError: This function has not been implemented in the specified language.

Edit: #160 filed to make this unnecessary.

@JarbasAl
Copy link
Collaborator

JarbasAl commented Nov 27, 2020

@ChanceNCounter i still think it's good for the file to exist, but with a TODO comment so devs can grep for TODO

but i agree with #160 , should not be mandatory

@ChanceNCounter
Copy link
Contributor

Squashing manually to preserve authorship, then merging

@ChanceNCounter ChanceNCounter merged commit 23e7b04 into MycroftAI:master Nov 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) hacktoberfest-accepted Credit a PR with Hacktoberfest goodness, without merging. sl relates to slovenian language
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants