Skip to content
This repository was archived by the owner on Sep 8, 2024. It is now read-only.

Switch language formatting and parsing to use Lingua Franca #2438

Merged
merged 7 commits into from
Jan 18, 2020
Merged

Switch language formatting and parsing to use Lingua Franca #2438

merged 7 commits into from
Jan 18, 2020

Conversation

forslund
Copy link
Collaborator

@forslund forslund commented Dec 23, 2019 •
edited
Loading

Description

This aims to replace the internal language parsing and formatting with the lingua franca module. It still aims to be backwards compatible so some logic remains in core along with all python modules.

  • It leaves all python intact but replaces every class and function defined under mycroft/util/lang with imports from lingua-franca.
  • util/(format|parse).py still contains some logic but mainly refers to lingua-franca
  • the data files in /mycroft/res has been completely removed.

How to test

Run core make sure things behaves. Do some explorative testing.

Contributor license agreement signed?

CLA [ Yes ]

@forslund forslund added the Status: Work in progress PR being actively worked on, not yet ready for review. label Dec 23, 2019
@devs-mycroft devs-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Dec 23, 2019
@forslund forslund force-pushed the feature/lingua-franca branch from eaa06c2 to d7f881f Compare December 23, 2019 05:50
@forslund forslund force-pushed the feature/lingua-franca branch 2 times, most recently from cf7ef9d to 15747d5 Compare January 13, 2020 09:59
@forslund forslund changed the title Lingua Franca [WIP] Switch language formatting and parsing to use Lingua Franca Jan 13, 2020
@forslund forslund removed the Status: Work in progress PR being actively worked on, not yet ready for review. label Jan 13, 2020
@forslund
Copy link
Collaborator Author

The coverage will drop and is acceptable.

@forslund forslund force-pushed the feature/lingua-franca branch from 15747d5 to faaba68 Compare January 13, 2020 10:15
@forslund forslund added tagged for review and potential merge Type: Refactoring and other improvements Improvement of code and documentation that does not alter functionality. labels Jan 13, 2020
@forslund forslund mentioned this pull request Jan 13, 2020
Merged
@forslund forslund force-pushed the feature/lingua-franca branch from faaba68 to 98a3f8a Compare January 13, 2020 15:40
chrisveilleux
chrisveilleux suggested changes Jan 13, 2020
View reviewed changes
Copy link
Member

@chrisveilleux chrisveilleux left a comment

Choose a reason for hiding this comment

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

I would like to see the interface between lingua_franca and mycroft_core rethought a bit. The API should be simple and the language function selection should be handled inside lingua_franca.

@forslund
Copy link
Collaborator Author

Yes it should but...until a major release we need to keep the core-api as is.

@ChanceNCounter
Copy link
Contributor

Seems to revert something about extract_datetime that breaks the weather skill. To repro: "Hey Mycroft, what's the weather like?" NoneType is not subscriptable

@forslund
Copy link
Collaborator Author

Thanks for reporting @ChanceNCounter totally reproducible. will look into and fix.

@forslund
Copy link
Collaborator Author

Hmm, seems like the lingua-franca behavior is the intended behavior: https://github.com/MycroftAI/mycroft-core/blob/dev/mycroft/util/parse.py#L248

@forslund
Copy link
Collaborator Author

Likely something similar to this fix: #2300

@forslund forslund force-pushed the feature/lingua-franca branch from 98a3f8a to ede37e1 Compare January 14, 2020 15:56
@forslund
Replace format and parse calls with lingua-franca
1b88db4
@forslund
Remove content from lang specific files
32666e9 
The files are kept for backwards compatibility but these just contains imports
of lingua-franca versions of variables
@forslund
Switch to v0.2.0 of lingua-franca
fe7a0d7
@forslund
Remove functions from format.py and parse.py
2cc90f3
@forslund
Add wrapper for extract_datetime
1b5b4f3
@forslund
Remove logic from format.py and parse.py
37caf78 
The functions are still there providing the docstrings but calling the
lingua_franca functions.

Note, there is a INTENTIONAL addition of an inconsistence. Making the
extract_datetime return todays date instead of None as is documented.
This needs to be removed as soon as the mycroft default skills have been
added.
@forslund
Remove test cases
f77fe9d 
The functionallity of lingua franca has been verified so the language
specific test cases aren't needed anymore. The base test cases for
english for format.py and parse.py is left as long as those remain as
part of the utils module.
@forslund forslund force-pushed the feature/lingua-franca branch from ede37e1 to f77fe9d Compare January 18, 2020 07:59
@forslund forslund merged commit 0b0f31d into dev Jan 18, 2020
@forslund forslund deleted the feature/lingua-franca branch January 18, 2020 08:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@JarbasAl JarbasAl Awaiting requested review from JarbasAl

@krisgesling krisgesling Awaiting requested review from krisgesling

1 more reviewer

@chrisveilleux chrisveilleux chrisveilleux approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) Type: Refactoring and other improvements Improvement of code and documentation that does not alter functionality.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@forslund @ChanceNCounter @chrisveilleux @devs-mycroft