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

Fix bug 1616918 - Enforce newline at end of Fluent strings when compa… #1574

Merged
merged 3 commits into from
Feb 28, 2020

Conversation

adngdb
Copy link
Collaborator

@adngdb adngdb commented Feb 27, 2020

…ring.

This solves a detection problem with simple Fluent strings, where the newline is missing from some Fluent string in history. This enforces trailing newlines, making the selector find the correct similar translation.

…ring.

This solves a detection problem with simple Fluent strings, where the newline is missing from some Fluent string in history. This enforces trailing newlines, making the selector find the correct similar translation.
@adngdb adngdb requested a review from mathjazz February 27, 2020 16:26
Copy link
Collaborator

@mathjazz mathjazz left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

I've also noticed we always show the Save button in the Source editor. Can we fix that, too? If it's non-trivial, it should be a followup bug (I assume source editor is not used very often).

@mathjazz
Copy link
Collaborator

(Unless you want to also fix the Source Editor, then I'll review again) :-)

@adngdb
Copy link
Collaborator Author

adngdb commented Feb 28, 2020

Yeah, I wanted to fix it because it was actually caused by this. Now it should work in all fluent cases. I've also added a bit more code to fix another issue with unsaved changes (I do it here because I find them as the initial translation is also used in the same translation selector, sorry for the review mess).

Note that I can reproduce the unsaved changes issue I talked about locally, not sure why it doesn't happen in the case you linked to.

@adngdb adngdb requested a review from mathjazz February 28, 2020 14:49
Copy link
Collaborator

@mathjazz mathjazz left a comment

Choose a reason for hiding this comment

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

Great work!

I also can't seem to reproduce https://bugzilla.mozilla.org/show_bug.cgi?id=1614590 anymore. Is this expected or a side effect?

// we want to turn the string into a Fluent message, as that's simpler
// to handle and less prone to errors. We do the same for each history
// entry.
let ftlMessage = fluent.parser.parseEntry(translation);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: I don't like that we're using two inconsistent names for the seamingly same concept: ftlMessage and fluentTranslation.

@adngdb adngdb merged commit 83dc3c6 into mozilla:master Feb 28, 2020
@adngdb adngdb deleted the 1616918-ftl-approve-submit branch February 28, 2020 16:06
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.

None yet

2 participants