Never migrate partial translations #44
Conversation
stasm
force-pushed
the
never-merge-partial
branch
from
f951d5c
to
c86b5b7
Compare
|
Also cc @flodolo. I ran the migration from bug 1424682 on all locales and encountered no errors. Granted, it means that some partial translations for some old inactive locales might be lost in the migration. |
I think that's fine. |
|
Yes, that's my thinking as well. I think we should prioritize being able to run migrations without errors over migrating strict 100% of what could be salvaged. |
stasm
force-pushed
the
never-merge-partial
branch
from
c86b5b7
to
5fe65e1
Compare
|
Confirming that migration runs without errors with this patch applied. Note: this creates files with message references that are not actually available. For example: category-general
.tooltiptext = { pane-general-title }But compare-locales catches those as WARNINGs, even if the message is a bit misleading: |
|
@flodolo, can you file bugs on each issue? Better wording, and message references to non-existing messages? |
This isn't caused by this patch. It's because the migration spec creates this message from nothing: FTL.Message(
id = FTL.Identifier('category-general'),
attributes = [
FTL.Attribute(
FTL.Identifier('tooltiptext'),
FTL.Pattern(
elements = [
FTL.Placeable(
expression = FTL.MessageReference(
id = FTL.Identifier(
'pane-general-title'
)
)
)
]
)
)
]
)In this case, nothing is supposed to be migrated. I don't think the migration code should check if the message has references and if they point to an existing messages (which is likely impossible to establish with certainty). |
Filed a bug for the first part, not sure I get the second. Is it about being able to fall back to English at run-time for them, or about the migration? Filed also |
There was a problem hiding this comment.
I'm not happy with how we test this. I've filed bug 1436014 on that.
Please remove the duplicated tests, and also nitpick on the docstrings a bit now. Happy to help with the rest later.
| {} | ||
| ) | ||
|
|
||
| def test_missing_string_in_one_of_attributes(self): |
tests/migrate/test_context.py
Outdated
| {} | ||
| ) | ||
|
|
||
| def test_missing_string_in_only_attribute(self): |
There was a problem hiding this comment.
Duplicate test, copy-n-paste error?
There was a problem hiding this comment.
Yes, thanks. I must have accidentally pasted twice.
| # Make sure all the dependencies are present in the current | ||
| # changeset. Partial migrations are not currently supported. | ||
| # See https://bugzilla.mozilla.org/show_bug.cgi?id=1321271 | ||
| available_deps = message_deps & changeset |
There was a problem hiding this comment.
I'd love to see some of this reflected in the docstrings of either merge_context or in_changeset. Re-reading both, I wonder if it'd be good to balance the two, for merge_context describing what it does in more detail, and in_changeset describing potential implementation details. Sounds like out-of-scope for this patch, but good docstrings help review. PS: I like the commit message, which I can't say on pr reviews :-(
Partial translations may break the AST because they produce `TextElements` with `None` values. For now, explicitly skip any transforms which depend on at least one missing legacy string. In the future we might be able to allow partial migrations in some situations, like migrating a subset of attributes of a message. See https://bugzilla.mozilla.org/show_bug.cgi?id=1321271.
stasm
force-pushed
the
never-merge-partial
branch
from
5fe65e1
to
922a983
Compare
Partial translations may break the AST because they produce
TextElementswithNonevalues. For now, explicitly skip any transforms which depend on at least one missing legacy string.In the future we might be able to allow partial migrations in some situations, like migrating a subset of attributes of a message. See https://bugzilla.mozilla.org/show_bug.cgi?id=1321271.