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 serving of backend translations #12453

Merged
merged 21 commits into from
Mar 1, 2018
Merged

Support serving of backend translations #12453

merged 21 commits into from
Mar 1, 2018

Conversation

emlove
Copy link
Contributor

@emlove emlove commented Feb 16, 2018

Description

This PR introduces the notion of backend-defined translations. When platforms are loaded, they will be able to register additional translation strings that can be served to clients. Fixes home-assistant/architecture#9

Still quite a ways from MVP, but I wanted to get some better visibility into our path forward and potentially some early feedback.

Source files

We'll manage our source files with git. Sources will be checked in adjacent to the component file. If the component is the main file for a Python package (i.e. __init__.py), the strings will be stored in a file named strings.json. Otherwise, they'll be stored in a file named strings.<platform_name>.json. Examples: (homeassistant/components/zwave/strings.json, homeassistant/components/sensor/strings.season.json)

Lokalise downloads

The Lokalise downloads will also be checked in to the repo, but shouldn't ever be edited except through the translation_download script. They'll be stored in a hidden directory .translations, also adjacent to the component. Similarly to the source files above, they'll be named with the format <language_code>.json, or <component_name>.<language_code>.json. Examples: (homeassistant/components/zwave/.translations/en.json, homeassistant/components/sensor/.translations/season.de.json)

Frontend PR: home-assistant/frontend#894

Docs PR: home-assistant/home-assistant.io#4787

Lokalise temporary project: https://lokalise.co/project/104514435a91b786c77a78.62627628/
image

TODO:

  • Register platform translations with the frontend component when platforms are first setup
  • Determine how translation sources will be checked in to the repo. (Equivalent of src/translations/en.json on the frontend.)
  • Determine strategy to synchronize / distribute full translation sets from Lokalize through to the PyPI package.
  • Tests
  • Create actual Lokalise project before merging
  • Add balloobbot keys for Lokalise in backend Travis CI job

Future PR:

EDIT: Migrated to home-assistant/frontend#498 for tracking

  • Developer docs
  • Allow clients to fetch translations for a single component
  • Notify clients when component with translations is set up
  • Determine backend fetch caching and strategy (Possibly include etags with the response)

@emlove emlove requested a review from a team as a code owner February 16, 2018 00:38
@@ -19,7 +19,8 @@
from homeassistant.components.http import HomeAssistantView
from homeassistant.components.http.auth import is_trusted_ip
from homeassistant.config import find_config_file, load_yaml_config_file
from homeassistant.const import CONF_NAME, EVENT_THEMES_UPDATED
from homeassistant.const import (

Choose a reason for hiding this comment

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

'homeassistant.const.EVENT_TRANSLATIONS_UPDATED' imported but unused

"""Return themes."""
hass = request.app['hass']

resources = hass.data[DATA_TRANSLATIONS].get(language)
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer if we structure this like we just restructured service descriptions: on demand.

So have this view look at which components are loaded (hass.config.components), look at their file names and see if they have files. For example, the light folder would look like:

  • __init__.py
  • services.yaml
  • strings.json
  • strings.nl.json

Bonus points if the frontend would be able to fetch the strings on a per component basis. So if it sees a string for component.light.setup.bla, it will only fetch the light strings from the backend.

output = {}
for key, value in data.items():
if isinstance(value, dict):
output.update(recursive_flatten('{}{}.'.format(prefix, key), value))

Choose a reason for hiding this comment

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

line too long (80 > 79 characters)

output = {}
for key, value in data.items():
if isinstance(value, dict):
output.update(recursive_flatten('{}{}.'.format(prefix, key), value))

Choose a reason for hiding this comment

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

line too long (80 > 79 characters)

Copy link
Contributor

@andrey-git andrey-git left a comment

Choose a reason for hiding this comment

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

We should either have a single file per component or break the world on duplicate keys.


@asyncio.coroutine
def get(self, request, language):
"""Return themes."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Update doc

platform = component.split('.', 1)[1]
filename = 'strings.{}.json'.format(platform)
else:
filename = 'strings.json'
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work for components without their own dir


os.makedirs("build", exist_ok=True)

json_util.save_json(os.path.join("build", "translations-upload.json"), translations)

Choose a reason for hiding this comment

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

line too long (88 > 79 characters)




def main():

Choose a reason for hiding this comment

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

too many blank lines (3)

#!/usr/bin/env python3
"""Merge all translation sources into a single JSON file."""
import glob
import itertools

Choose a reason for hiding this comment

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

'itertools' imported but unused

name = component


module = get_component(component)

Choose a reason for hiding this comment

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

too many blank lines (2)


os.makedirs("build", exist_ok=True)

json_util.save_json(os.path.join("build", "translations-upload.json"), translations)

Choose a reason for hiding this comment

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

line too long (88 > 79 characters)




def main():

Choose a reason for hiding this comment

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

too many blank lines (3)

#!/usr/bin/env python3
"""Merge all translation sources into a single JSON file."""
import glob
import itertools

Choose a reason for hiding this comment

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

'itertools' imported but unused

name = component


module = get_component(component)

Choose a reason for hiding this comment

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

too many blank lines (2)

@emlove emlove changed the title [WIP] Support serving of backend translations Support serving of backend translations Feb 25, 2018
@balloob
Copy link
Member

balloob commented Feb 26, 2018

I think that we should only accept the format components/<component name>/strings.json.

Edit: I misread. You used the word component but meant platform. For platforms we should indeed stick to components/<component name>/strings.<platform name>.json

@balloob balloob removed the small-pr PRs with less than 30 lines. label Feb 26, 2018
@balloob
Copy link
Member

balloob commented Feb 26, 2018

(btw no access to temporary lokalise project)

if domain not in resources:
resources[domain] = {}

# Add the translations for this component to the domain resources.
Copy link
Member

Choose a reason for hiding this comment

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

Something I was thinking about the other day, especially now that we're doing more entity based config: should we store platform as an attribute in the state machine?

Copy link
Member

Choose a reason for hiding this comment

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

This one is going to get massive for sensor component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The platform as an attribute would certainly be a big help here. It would let us completely avoid this problem and keep the translations namespaced separately. That's something we could add later though if we decide to make it available, as the conflict only exists in the API response. All of our translations at rest are stored separately per platform.

And FWIW this will only return translations for loaded components, so although we'll have lots of translations merged, each instance should only serve the translations it's using.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The platform as an attribute would certainly be a big help here. It would let us completely avoid this problem and keep the translations namespaced separately. That's something we could add later though if we decide to make it available, as the conflict only exists in the API response. All of our translations at rest are stored separately per platform.

And FWIW this will only return translations for loaded components, so although we'll have lots of translations merged, each instance should only serve the translations it's using.

components = hass.config.components

# Load missing files
missing = set()
Copy link
Member

@balloob balloob Feb 26, 2018

Choose a reason for hiding this comment

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

FYI Sets can be subtracted from one another.

missing = hass.config.components - set(translation_cache)

load_translations_files, missing)

# Update cache
for component in components:
Copy link
Member

Choose a reason for hiding this comment

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

Are you calculating missing again here?

# Update cache
for component in components:
if component not in translation_cache:
json_file = component_translation_file(component, language)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we can send a dictionary mapping component -> file to load_translation_files so we don't have to generate all the keys again.

async def async_get_translations(hass, language):
"""Return all backend translations."""
if language == 'en':
resources = await async_get_component_resources(hass, language)
Copy link
Member

Choose a reason for hiding this comment

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

You can move this before the if and turn the else into an if language != 'en'

Copy link
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

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

It looks great. I realize I was a bit too picky with my comments, you can ignore them.

@emlove
Copy link
Contributor Author

emlove commented Feb 26, 2018

No problem, it's all great advice per usual.

I thought I made the Lokalise project public, but maybe it's because it's a trial. In any case, there's not much there. I mostly just linked it for an example of how the namespacing will look in Lokalise. I attached a screenshot to the PR description.

I'm going to go ahead and get the docs ready and start setting up the live Lokalise project. It'll probably be a few days before I can get to it.

@emlove
Copy link
Contributor Author

emlove commented Mar 1, 2018

OK, I've got the project spinned up on Lokalise. The last step should be to get the balloobbot credentials copied from the polymer build job to the backend master build job in Travis. After that we can push the button and see if it works.

I've also put up a small PR to update the translation docs to include a link to the new project. home-assistant/home-assistant.io#4787

@balloob
Copy link
Member

balloob commented Mar 1, 2018

Added LOKALISE_TOKEN env var to the Travis builds of this repo.

@balloob balloob merged commit b434ffb into dev Mar 1, 2018
@balloob balloob deleted the backend-translations branch March 1, 2018 03:31
@balloob
Copy link
Member

balloob commented Mar 1, 2018

🤞

@balloob balloob mentioned this pull request Mar 9, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Jul 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants