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

Adds rudimentary Dutch parsing support #2385

Merged
merged 1 commit into from Nov 15, 2019
Merged

Conversation

@mikewoudenberg
Copy link
Contributor

mikewoudenberg commented Nov 13, 2019

Description

Adds rudimentary Dutch language support to Mycroft parser as mentioned in #2380

How to test

  • Run the unit tests
  • Change Mycroft's language to Duth and experiment with any skill using the internal parser. These skills use the parser:
    • The cut up skill
    • The dice-skill
    • The wolfram alpha skill
    • The mycroft timer skill
    • The mycroft wink iot skill
    • The alarm skill
    • The audio record skill
    • The date-time skill
    • The reminder skill
    • The volume skill
    • The weather skill

Contributor license agreement signed?

CLA [x]

@devs-mycroft

This comment has been minimized.

Copy link
Collaborator

devs-mycroft commented Nov 13, 2019

Hello, @mikewoudenberg, thank you for helping with the Mycroft project! We welcome everyone
into the community and greatly appreciate your help as we work to build an AI
for Everyone.

To protect yourself, the project, and users of Mycroft technologies we require
a Contributor Licensing Agreement (CLA) before accepting any code
contribution. This agreement makes it crystal clear that along with your
code you are offering a license to use it within the confines of this project.
You retain ownership of the code, this is just a license.

Please visit https://mycroft.ai/cla to initiate this one-time signing. Thank
you!

Copy link
Member

forslund left a comment

Thank you for this.

Looks good as far as I can see. A couple of nit-picks with the year on the copyright notice otherwise I trust the tests. Maybe @j1nx could take a look since he seems to know dutch.

mycroft/util/lang/common_data_nl.py Outdated Show resolved Hide resolved
mycroft/util/lang/parse_nl.py Outdated Show resolved Hide resolved
Copy link

j1nx left a comment

AMAZING JOB! Well done. I will try to make the time to pull in this PR and give it a go to check the operational side. Only looked at the translation side for now.

mycroft/util/lang/common_data_nl.py Outdated Show resolved Hide resolved
mycroft/util/lang/parse_nl.py Outdated Show resolved Hide resolved
mycroft/util/lang/parse_nl.py Outdated Show resolved Hide resolved
mycroft/util/lang/parse_nl.py Outdated Show resolved Hide resolved
mycroft/util/lang/parse_nl.py Outdated Show resolved Hide resolved
mycroft/util/lang/parse_nl.py Outdated Show resolved Hide resolved
@mikewoudenberg mikewoudenberg force-pushed the mikewoudenberg:dev branch from 755f81f to cc5631c Nov 14, 2019
@krisgesling krisgesling added CLA: Yes and removed CLA: Needed labels Nov 14, 2019
@krisgesling

This comment has been minimized.

Copy link
Contributor

krisgesling commented Nov 14, 2019

Thanks Mike, CLA received. So this is good to go if all requests have been addressed.

@forslund

This comment has been minimized.

Copy link
Member

forslund commented Nov 14, 2019

When @j1nx thinks this is good to go I'll hit the Merge button!

@j1nx

This comment has been minimized.

Copy link

j1nx commented Nov 14, 2019

Looks fine by me. Meaning looks fine translation-wise from a second Dutch guy's opinion / review.

Not 100% sure if all the big numbers translates correctly, but to be honest, didn't even know they exists ;)

Glad somebody picked this up, so am all for merging. This means we can use Google STT/TTS and change to Dutch.

Thank you very much for this work @mikewoudenberg

@forslund

This comment has been minimized.

Copy link
Member

forslund commented Nov 15, 2019

Many Thanks for this, Merging now!

@forslund forslund merged commit 17840de into MycroftAI:dev Nov 15, 2019
3 checks passed
3 checks passed
:-) Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 56.034%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.