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

Have alignments placed if HTML is on #296

Merged
merged 2 commits into from
Jan 3, 2022

Conversation

jerinphilip
Copy link
Contributor

@jerinphilip jerinphilip commented Jan 2, 2022

Fixes one half of #270.

#271 tries to provide a sanitizeOptions, which is perhaps more extensible. But such a function will have to be called at several sites increasing verbosity is my conclusion after experimenting in jerinphilip#28.

Instead, we just pick-up ResponseOptions.HTML and place alignments with an or switch if HTML is on, irrespective of whatever value was given for Alignments.

This way a client can go from:

$ cat | bergamot translate -m en-es-tiny --html=1 --alignment=1 
<a href="Tintin">Tintin</a>'s dog is a good dog.
El perro de <a href="Tintin">Tintín</a> es un buen perro.

to

$ cat | bergamot translate -m en-es-tiny --html=1 
<a href="Tintin">Tintin</a>'s dog is a good dog.
El perro de <a href="Tintin">Tintín</a> es un buen perro.

(This is tested with python-cmdline).

Should work similarly with ResponseOptions for WebAssembly as well.

@jelmervdl
Copy link
Member

I prefer this solution to #271. It might be worth to nuke response.alignments after HTML has been restored if options.alignments == false to reduce the amount of data that crosses the wasm -> page context bridge.

@jerinphilip
Copy link
Contributor Author

This simplifies existing ResponseOptions and any future edits can be done without changing external API. Supported behaviour is HTML means alignments will be computed under the hood for HTML. Merging therefore.

Caveat emptor, especially if the user intends on using alignment-matrix through only the HTML options in Response object.

@jerinphilip jerinphilip merged commit 81c2192 into browsermt:main Jan 3, 2022
@jerinphilip jerinphilip deleted the html-implies-alignments branch January 3, 2022 12:27
@jelmervdl
Copy link
Member

Caveat emptor, especially if the user intends on using alignment-matrix through only the HTML options in Response object.

This is why I would prefer to clear response.alignments if !options.alignments so that this is only available if options.alignments == true, regardless of whether options.html is true or false.

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