Skip to content

Bug 922577 - Do not leak attributes between translations #73

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

Merged
merged 1 commit into from
Sep 29, 2017

Conversation

stasm
Copy link
Contributor

@stasm stasm commented Sep 6, 2017

@stasm stasm requested review from zbraniecki and Pike September 6, 2017 12:47
@zbraniecki
Copy link
Collaborator

If I understand the patch correctly, you'd like to forbid localizable attributes to be placed in the source.
It seems like it could surprise a developer - maybe we should log a warning if that happens?

@stasm
Copy link
Contributor Author

stasm commented Sep 7, 2017

If I understand the patch correctly, you'd like to forbid localizable attributes to be placed in the source.

I think there are a few things we could do to improve the developer experience:

  • warn when a non-localizable element is present in source (it will be removed),
  • warn when a localizable attribute is removed from the source element,
  • warn when a localizable attribute us removed from a source element's child.

Do you think we should do all of them in this PR?

@zbraniecki
Copy link
Collaborator

Maybe we should even start with throwing?

@stasm
Copy link
Contributor Author

stasm commented Sep 7, 2017

I think throwing is only a viable option for the first use-case from my comment above. In the other two we could break the UI on retranslation if the new localization is missing an attribute.

@stasm stasm merged commit 64ef212 into projectfluent:master Sep 29, 2017
@stasm stasm deleted the 922577-do-not-leak-attrs branch September 29, 2017 12:03
@Pike Pike removed their request for review May 16, 2018 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants