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

Add Config handling for language modules #74

Merged
merged 8 commits into from
Sep 16, 2022
Merged

Conversation

NeonDaniel
Copy link
Member

Update Language factory to pass module config
Fix typo in TranslationFactory inig
Add default config to base Language classes

Fix typo in TranslationFactory inig
Add default config to base Language classes
@NeonDaniel NeonDaniel requested a review from JarbasAl August 26, 2022 00:34
@codecov
Copy link

codecov bot commented Aug 26, 2022

Codecov Report

❗ No coverage uploaded for pull request base (dev@bbbd071). Click here to learn what that means.
The diff coverage is n/a.

@@          Coverage Diff          @@
##             dev     #74   +/-   ##
=====================================
  Coverage       ?   0.00%           
=====================================
  Files          ?      40           
  Lines          ?    2661           
  Branches       ?       0           
=====================================
  Hits           ?       0           
  Misses         ?    2661           
  Partials       ?       0           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@NeonDaniel NeonDaniel requested a review from JarbasAl August 26, 2022 23:29
@JarbasAl
Copy link
Member

couple nitpicks that can be ignored, this looks good and much cleaner!

Copy link
Member

@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.

same comment as above for all template classes, configs should only be loaded in factory classes and passed as arguments, the templates should stay "pure" and fully independent objects for usage outside mycroft context

@NeonDaniel NeonDaniel requested a review from JarbasAl August 29, 2022 23:14
"""
config = config or Configuration()
lang = config.get('lang') or Configuration().get('lang')
config = (config.get('intentBox', {}).get(section) or config.get(section)
Copy link
Member

Choose a reason for hiding this comment

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

should this be behind some other check? will merge for now but we should revisit since this allows any config to live under intentBox section and we likely want to throw an error in that case

@JarbasAl JarbasAl added bug Something isn't working refactor code improvements with no functional changes labels Sep 16, 2022
@JarbasAl JarbasAl merged commit 48e8e0d into dev Sep 16, 2022
@NeonDaniel NeonDaniel deleted the FEAT_DefaultLanguageConfig branch February 25, 2023 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working refactor code improvements with no functional changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants