Improved localization through google getMsg()

[taimir]

This is an improved version of my getMsg() proposal for the localization in dashboard. It differs from #528 in the following aspects:

• I tried to parallelize as much of the code as possible. Previously, the build-frontend was called once for each locale, which is an overkill. Now build-frontend is called only once and the result is then multiDest-ed into all the arch x locale directories.
• The localization is then achieved by replacing only the app.js files (instead of calling the whole build-frontend like before).
• The different localized versions of the app.js files are built via gulp scripts:prod into separate directories under .tmp. NOTE: I could not make this step parallel, because gulp-closure-compiler cannot be run in parallel. I know the exact point of failure, it is here. Basically its an issue with the temporary file used to hold the output of the closure-compiler.
• Instead of using merge-stream and async like before, I now only use multiDest and standard gulp-tasks defined on the fly, which I execute with run-sequence. That way we have the benefit of no third party dependencies and we can directly see the actual tasks being executed in the console (with the nice gulp coloring, etc.).
• The LocaleHandler backend component now first compares the Accept Language header to a list of predefined locales before serving anything (security measure).

@bryk PTAL, I tried to improve upon all of your remarks. Do you think the LocaleHandler is now secure enough? What do we do with the scripts:prod task, I cannot run it in parallel because of the gulp-closure-compiler, but there is no way around calling it once for each locale.

---

This change is

[codecov-io]

Current coverage is 95.22%

Merging #725 into master will increase coverage by 0.43%

1. File ...ereader_directive.js (not in diff) was modified. more
   • Misses -6
   • Partials 0
   • Hits +6

@@             master       #725   diff @@
==========================================
  Files           175        175          
  Lines          1360       1361     +1   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1289       1296     +7   
+ Misses           71         65     -6   
  Partials          0          0          

Powered by Codecov. Last updated by d2c14c7...bf19887

[taimir]

@floreks @maciaszczykm Can you have a look at the travis build? I see some strange error during the integration tests, but the build turns out green? Is this normal?

[floreks]

It is normal. They are turned off, but still executed.

[bryk]

PTAL, I tried to improve upon all of your remarks. Do you think the LocaleHandler is now secure enough?

Yes, looks way better now :)

---

Reviewed 2 of 19 files at r1.
Review status: 2 of 19 files reviewed at latest revision, 4 unresolved discussions.

---

a discussion (no related file):
A few initial comments. Will test now.

---

a discussion (no related file):
Btw, please rebase, since we've had changes in our build system recently.

---

src/app/backend/handler/localehandler.go, line 47 [r1] (raw file):

func LocaleHandler(w http.ResponseWriter, r *http.Request) {
  if r.URL.EscapedPath() == "/" || r.URL.EscapedPath() == "/index.html" {
      // do not store the html page in the cache

Why?

---

src/app/backend/handler/localehandler.go, line 86 [r1] (raw file):

}

func getSupportedLocales() ([]string, error) {

How about loading the configuration at server startup? Currently you load the file on every request to index.html, which is suboptimal.

When the backend loads all configuration at startup you can be sure that once it starts correctly, it'll function correctly. This is a huge benefit in a production system. Here, when you do config loading at runtime, everything may break at any time.

What do you think?

cc @floreks

---

Comments from Reviewable

[bryk]

Why dist/amd64/public/locale_conf.json is in public/? It should be in dist/amd64/locale_conf.json, i.e., the same dir that the binary is :)

[taimir]

I overlooked the exact position of the binary. Thanks, I'll place it into the arch directory directly then.

[bryk]

Review status: 2 of 19 files reviewed at latest revision, 13 unresolved discussions.

---

a discussion (no related file):
A few more comments. Feel free to discuss. Overall, I'm impressed and hope to get this merged soon.

---

build/build.js, line 26 [r1] (raw file):

import gulpUseref from 'gulp-useref';
import path from 'path';
import RevAll from 'gulp-rev-all';

Name it gulpRevAll. This is how other gulp plugins are called most often :)

---

build/build.js, line 126 [r1] (raw file):

      .pipe(gulpIf('**/vendor.css', gulpMinifyCss()))
      .pipe(gulpIf('**/vendor.js', gulpUglify({preserveComments: uglifySaveLicense})))
      .pipe(gulpIf(['**/*.js', '**/*.css'], gulpRev()))

Why do you remove it from here? Shouldn't this just work?

---

build/build.js, line 203 [r1] (raw file):

    let localizedOutputDirs =
        outputDirs.map((outputDir) => { return path.join(outputDir, translation.key, 'static'); });
    gulp.task(`localize:${translation.key}`, function(doneFn) {

Can you not generate gulp.tasks and use plain functions/streams instead? Additional thing (apart from mentioned by me in the other comment) is that gulp.tasks are for organizing dependencies. And the task localize:en cannot has no formal dependencies, yet it requires some prerequisites.

cc @floreks

---

build/build.js, line 230 [r1] (raw file):

function assets(outputDirs) {
  // build for each language and cross-arch
  let localizedOutputDirs = outputDirs.reduce((localizedDirs, outputDir) => {

I see a copy &paste from distFiles function. Can you extract this?

---

build/build.js, line 245 [r1] (raw file):

 */
function icons(outputDirs) {
  let localizedOutputDirs = outputDirs.reduce((localizedDirs, outputDir) => {

Same copy & paste :)

---

build/multidest.js, line 21 [r1] (raw file):

 * Utility function for specifying multiple gulp.dest destinations.
 * @param {string|!Array<string>} outputDirs destinations for the gulp dest function calls
 * @param {function(?Error=)} doneFn - Callback.

Say that this is optional with |undefined type and name opt_doneFn :)

---

build/script.js, line 120 [r1] (raw file):

  // TODO (taimir) : do not run the tasks sequentially once
  // gulp-closure-compiler can be run in parallel
  gulp.task(

Hmm... I this looks like a wrong pattern. Basically, you're creating gulp.tasks on the fly. So I see a task in console "scripts:en" but I cannot run it with gulp scripts:en. How about you make it a normal function that reutrns a stream that you run sequentially? This would have additional benefit of easy parallelism when closure compiler starts working correctly.

---

docs/design/i18n.md, line 18 [r1] (raw file):

  |   +-- en/
  |   |   +-- public/
  |   |       +-- index.html <-- uses the assets from static

How about we have a minimalistic folder structure where only files that need localization are copied? I.e., dist/amd64/public/ and here index.en.html and in static app.en.js plus other files untouched?

This would unbloat our binary where we copy everything N times for all languages.

Is this hard to implement?
cc @floreks

---

Comments from Reviewable

[taimir]

Review status: 2 of 19 files reviewed at latest revision, 13 unresolved discussions.

---

build/build.js, line 126 [r1] (raw file):

Previously, bryk (Piotr Bryk) wrote…

Why do you remove it from here? Shouldn't this just work?

Because I am replacing the app.js file afterwards, to actually localize this particular language. So I had to do the revision at a later step, and decided to do it all at once with revAll. Do you find this problematic?

---

build/build.js, line 203 [r1] (raw file):

Previously, bryk (Piotr Bryk) wrote…

Can you not generate gulp.tasks and use plain functions/streams instead? Additional thing (apart from mentioned by me in the other comment) is that gulp.tasks are for organizing dependencies. And the task localize:en cannot has no formal dependencies, yet it requires some prerequisites.

cc @floreks

OK, the main reason I went for this solution is that gulp tasks are a core functionality and are pretty much guaranteed to always work properly. Nevertheless, if you really think this is a bad practice, I can revert to some plugin like 'merge-stream' or 'async' for this.

Just so that I can understand your point better: is your concern that the definition of the gulp tasks is "hidden" in the code? Basically that the visibility of those tasks is obstructed. Or is there some other functional disadvantage I am not considering.

---

docs/design/i18n.md, line 18 [r1] (raw file):

Previously, bryk (Piotr Bryk) wrote…

How about we have a minimalistic folder structure where only files that need localization are copied? I.e., dist/amd64/public/ and here index.en.html and in static app.en.js plus other files untouched?

This would unbloat our binary where we copy everything N times for all languages.

Is this hard to implement?
cc @floreks

I have to think this through, but at a first glance I think it should be possible with a few adaptations of the code.

---

src/app/backend/handler/localehandler.go, line 47 [r1] (raw file):

Previously, bryk (Piotr Bryk) wrote…

Why?

If I remember correctly: if the index.html file gets cached, a user-trigger locale change (e.g. clicking on "switch language") will not lead to the index.html being served once more, and the locale will not change. I had experienced this in chrome while testing the locales out.

---

src/app/backend/handler/localehandler.go, line 86 [r1] (raw file):

Previously, bryk (Piotr Bryk) wrote…

How about loading the configuration at server startup? Currently you load the file on every request to index.html, which is suboptimal.

When the backend loads all configuration at startup you can be sure that once it starts correctly, it'll function correctly. This is a huge benefit in a production system. Here, when you do config loading at runtime, everything may break at any time.

What do you think?

cc @floreks

Sounds like an improvement to me!

---

Comments from Reviewable

[bryk]

Review status: 2 of 19 files reviewed at latest revision, 18 unresolved discussions.

---

build/build.js, line 126 [r1] (raw file):

Previously, taimir (Atanas Mirchev) wrote…

Because I am replacing the app.js file afterwards, to actually localize this particular language. So I had to do the revision at a later step, and decided to do it all at once with revAll. Do you find this problematic?

Ok sounds good

---

build/build.js, line 142 [r1] (raw file):

/**
 * Builds production version of the frontend application.

The documentation needs to be updated.

---

build/build.js, line 147 [r1] (raw file):

 *  1. Vendor CSS and JS files are concatenated and minified.
 *  2. index.html is minified.
 *  4. Everything is saved in the .tmp/dist directory, ready to be localized and revisioned.

Wrong bullet number :)

---

build/build.js, line 151 [r1] (raw file):

 * @return {stream}
 */
function distributeFiles(outputDirs) {

The name of this function does not say much. Do you have any idea for something more verbose? Like what files or replace distribute with something specific.

---

build/build.js, line 194 [r1] (raw file):

/**
 * Replaces the main app.js proprietary logic with a localized version

This comment is false, isn't it? This just copies files from A to B?

---

build/build.js, line 203 [r1] (raw file):

Previously, taimir (Atanas Mirchev) wrote…

OK, the main reason I went for this solution is that gulp tasks are a core functionality and are pretty much guaranteed to always work properly. Nevertheless, if you really think this is a bad practice, I can revert to some plugin like 'merge-stream' or 'async' for this.

Just so that I can understand your point better: is your concern that the definition of the gulp tasks is "hidden" in the code? Basically that the visibility of those tasks is obstructed. Or is there some other functional disadvantage I am not considering.

The main concern is that I cannot run gulp localize:en, which is very uncommon for gulp. If you just insert this in a normal JS function, this would do the same, but there will be no gulp tasks for this.

Finally, it is not "very bad". It is just a pattern that I'd like us to avoid when possible :)

---

docs/design/i18n.md, line 18 [r1] (raw file):

Previously, taimir (Atanas Mirchev) wrote…

I have to think this through, but at a first glance I think it should be possible with a few adaptations of the code.

Yeah, if it is easy, do this :) If you're going to spend too much time on this, then not.

---

src/app/backend/handler/localehandler.go, line 47 [r1] (raw file):

Previously, taimir (Atanas Mirchev) wrote…

If I remember correctly: if the index.html file gets cached, a user-trigger locale change (e.g. clicking on "switch language") will not lead to the index.html being served once more, and the locale will not change. I had experienced this in chrome while testing the locales out.

All right. Can you add this comment to the code here. Long explanation why this decision was made :)

---

src/app/backend/handler/localehandler.go, line 55 [r1] (raw file):

}

func determineLocalizedDir(locale string) string {

Tests for this?

---

Comments from Reviewable

[taimir]

Review status: 2 of 20 files reviewed at latest revision, 14 unresolved discussions.

---

build/build.js, line 26 [r1] (raw file):

Previously, bryk (Piotr Bryk) wrote…

Name it gulpRevAll. This is how other gulp plugins are called most often :)

Done.

---

build/build.js, line 142 [r1] (raw file):

Previously, bryk (Piotr Bryk) wrote…

The documentation needs to be updated.

Done.

---

build/build.js, line 147 [r1] (raw file):

Previously, bryk (Piotr Bryk) wrote…

Wrong bullet number :)

Done.

---

build/build.js, line 151 [r1] (raw file):

Previously, bryk (Piotr Bryk) wrote…

The name of this function does not say much. Do you have any idea for something more verbose? Like what files or replace distribute with something specific.

Done.

---

build/build.js, line 194 [r1] (raw file):

Previously, bryk (Piotr Bryk) wrote…

This comment is false, isn't it? This just copies files from A to B?

Done.

---

build/build.js, line 230 [r1] (raw file):

Previously, bryk (Piotr Bryk) wrote…

I see a copy &paste from distFiles function. Can you extract this?

Done.

---

build/build.js, line 245 [r1] (raw file):

Previously, bryk (Piotr Bryk) wrote…

Same copy & paste :)

Done.

---

build/multidest.js, line 21 [r1] (raw file):

Previously, bryk (Piotr Bryk) wrote…

Say that this is optional with |undefined type and name opt_doneFn :)

Done.

---

build/script.js, line 120 [r1] (raw file):

Previously, bryk (Piotr Bryk) wrote…

Hmm... I this looks like a wrong pattern. Basically, you're creating gulp.tasks on the fly. So I see a task in console "scripts:en" but I cannot run it with gulp scripts:en. How about you make it a normal function that reutrns a stream that you run sequentially? This would have additional benefit of easy parallelism when closure compiler starts working correctly.

Done.

---

src/app/backend/handler/localehandler.go, line 55 [r1] (raw file):

Previously, bryk (Piotr Bryk) wrote…

Tests for this?

Done. See below.

---

Comments from Reviewable

[taimir]

@bryk I'm back at office, just had some time to pick this up again :) PTAL now, I moved the configuration of the LocaleHandler in the very beginning.

I'll see if I could make the folder structure "minimalistic" as you noted in one of your previous comments. Please check if you see any more issues.

Next step would now be to replace all text across dashboard with getMsg() variables. Should we keep those directly in the controllers, like the example in this PR? I'd like to know your opinions on this. @digitalfishpond @floreks

[bryk]

Next step would now be to replace all text across dashboard with getMsg() variables. Should we keep those directly in the controllers, like the example in this PR? I'd like to know your opinions on this. @digitalfishpond @floreks

Please create a separate issue to talk about this.

I'll see if I could make the folder structure "minimalistic" as you noted in one of your previous comments. Please check if you see any more issues.

Please create a separate issue and separate PR for this. This PR is good for now :)

---

Reviewed 9 of 19 files at r1, 3 of 7 files at r3, 1 of 2 files at r4.
Review status: 15 of 20 files reviewed at latest revision, 6 unresolved discussions.

---

a discussion (no related file):
There are errors thrown by the backend on gulp serve:prod. Can you fix them?

W0517 16:28:38.302236  139005 localehandler.go:50] Error when loading the localization configuration. Dashboard will not be localized. open ./locale_conf.json: no such file or directory

---

a discussion (no related file):
Last cosmetic comments and let's merge.

---

package.json, line 45 [r4] (raw file):

    "gulp-rev": "~7.0.0",
    "gulp-rev-all": "~0.8.24",
    "gulp-rev-replace": "~0.4.3",

Do we need this dependency anymore?

---

build/build.js, line 26 [r1] (raw file):

Previously, taimir (Atanas Mirchev) wrote…

Done.

Start with small letter, like: gulpRevAll. It is a variable and style here :) We love to be consistent here.

---

build/build.js, line 194 [r1] (raw file):

Previously, taimir (Atanas Mirchev) wrote…

Done.

I still think the comment is invalid here. This function copies files from A to B and does not do any "Replaces the main app.js logic with a localized version..."

---

build/postinstall.sh, line 26 [r4] (raw file):

if ! [ -a "./.tools/xtbgenerator/bin/XtbGenerator.jar" ]
then
  (cd ./.tools/; git clone https://github.com/kuzmisin/xtbgenerator)

Can you clone this at specific SHA version? We dont want to be broken when xtbgenerator gets broken. In general, it is better to have versioned dependencies than not.

---

Comments from Reviewable

[taimir]

Review status: 15 of 20 files reviewed at latest revision, 6 unresolved discussions.

---

a discussion (no related file):

Previously, bryk (Piotr Bryk) wrote…

There are errors thrown by the backend on gulp serve:prod. Can you fix them?

W0517 16:28:38.302236  139005 localehandler.go:50] Error when loading the localization configuration. Dashboard will not be localized. open ./locale_conf.json: no such file or directory
```</details>
Hm, strange ... this should not happen (hence the warning). Let me see.

---

package.json, line 45 [r4] (raw file):

Previously, bryk (Piotr Bryk) wrote…

Do we need this dependency anymore?

Thanks! No, indeed we do not.

---

build/build.js, line 26 [r1] (raw file):

Previously, bryk (Piotr Bryk) wrote…

Start with small letter, like: gulpRevAll. It is a variable and style here :) We love to be consistent here.

GulpRevAll is an object (in contrast to most of the other gulp plugins) and usually is to be initialized with new GulpRevAll(). Our linter complains to me that it should be with a capital letter because of this, hence why I couldn't rename it to a small-letter camel case. Should I work around this?

---

build/build.js, line 194 [r1] (raw file):

Previously, bryk (Piotr Bryk) wrote…

I still think the comment is invalid here. This function copies files from A to B and does not do any "Replaces the main app.js logic with a localized version..."

But it does copy the translated app.js files from .tmp/i18n/<locale> for each locale and places them into the <arch>/<locale> directories in .tmp/dist, where I have previously put the standard app.js just as a temporary placeholder. Or you mean this is way too specific internal stuff which should not be reflected in the comment?

---

build/postinstall.sh, line 26 [r4] (raw file):

Previously, bryk (Piotr Bryk) wrote…

Can you clone this at specific SHA version? We dont want to be broken when xtbgenerator gets broken. In general, it is better to have versioned dependencies than not.

Sure :) thanks for the tip

---

Comments from Reviewable

[taimir]

Review status: 15 of 20 files reviewed at latest revision, 6 unresolved discussions.

---

a discussion (no related file):

Previously, taimir (Atanas Mirchev) wrote…

Hm, strange ... this should not happen (hence the warning). Let me see.

Can you describe how to reproduce this error? It doesn't happen on my machine.

I cloned my fork into a brand new folder, switched to this branch (i18n-get-message-improved), ran npm install, gulp local-up-cluster and gulp serve:prod and the localization was fine (the backend picked up locale-conf.json)...

Maybe do gulp clean before you test?

---

Comments from Reviewable

[taimir]

Review status: 15 of 20 files reviewed at latest revision, 6 unresolved discussions.

---

package.json, line 45 [r4] (raw file):

Previously, taimir (Atanas Mirchev) wrote…

Thanks! No, indeed we do not.

Done.

---

build/postinstall.sh, line 26 [r4] (raw file):

Previously, taimir (Atanas Mirchev) wrote…

Sure :) thanks for the tip

Done.

---

Comments from Reviewable

[taimir]

Review status: 15 of 20 files reviewed at latest revision, 6 unresolved discussions.

---

a discussion (no related file):

Previously, taimir (Atanas Mirchev) wrote…

Can you describe how to reproduce this error? It doesn't happen on my machine.

I cloned my fork into a brand new folder, switched to this branch (i18n-get-message-improved), ran npm install, gulp local-up-cluster and gulp serve:prod and the localization was fine (the backend picked up locale-conf.json)...

Maybe do gulp clean before you test?

Hey, the warning which you had comes up in development, when you run `gulp serve` instead of `gulp serve:prod`. It is because the `locale-conf.json` file that the backend parses to determine the supported locales is not in the folder of the binary (because it's not called from the dist folder, since it's development and not production). In development we don't use the backend as a fileserver so it all works out fine, but we still do start the backend. Should I do anything special about this?

---

Comments from Reviewable

[bryk]

Reviewed 1 of 19 files at r1, 1 of 2 files at r4, 1 of 3 files at r5.
Review status: 18 of 20 files reviewed at latest revision, 5 unresolved discussions.

---

a discussion (no related file):

In development we don't use the backend as a fileserver so it all works out fine, but we still do start the backend. Should I do anything special about this?

Can you just copy the locale file to serve dir?

---

a discussion (no related file):

Previously, bryk (Piotr Bryk) wrote…

Last cosmetic comments and let's merge.

PTAL :)

---

package.json, line 45 [r4] (raw file):

Previously, taimir (Atanas Mirchev) wrote…

Done.

You did not remove the gulp-rev dependency. Or am I missing something?

---

build/build.js, line 26 [r1] (raw file):

Previously, taimir (Atanas Mirchev) wrote…

GulpRevAll is an object (in contrast to most of the other gulp plugins) and usually is to be initialized with new GulpRevAll(). Our linter complains to me that it should be with a capital letter because of this, hence why I couldn't rename it to a small-letter camel case. Should I work around this?

Oh, that's fine. I didn't notice this.

---

build/build.js, line 194 [r1] (raw file):

Previously, taimir (Atanas Mirchev) wrote…

But it does copy the translated app.js files from .tmp/i18n/<locale> for each locale and places them into the <arch>/<locale> directories in .tmp/dist, where I have previously put the standard app.js just as a temporary placeholder. Or you mean this is way too specific internal stuff which should not be reflected in the comment?

Now the comment is nice :)

---

Comments from Reviewable

[taimir]

Review status: 18 of 20 files reviewed at latest revision, 4 unresolved discussions.

---

a discussion (no related file):

Previously, bryk (Piotr Bryk) wrote…

In development we don't use the backend as a fileserver so it all works out fine, but we still do start the backend. Should I do anything special about this?

Can you just copy the locale file to serve dir?

OKs, I'm on it

---

package.json, line 45 [r4] (raw file):

Previously, bryk (Piotr Bryk) wrote…

You did not remove the gulp-rev dependency. Or am I missing something?

Sorry about that :S I forgot to `git add` it it seems

---

Comments from Reviewable

[bryk]

Review status: 18 of 20 files reviewed at latest revision, 4 unresolved discussions.

---

a discussion (no related file):

Previously, taimir (Atanas Mirchev) wrote…

OKs, I'm on it

Ping me when ready for rereview. I'm eager to merge this :)

---

Comments from Reviewable

[taimir]

Review status: 18 of 21 files reviewed at latest revision, 5 unresolved discussions.

---

build/serve.js, line 166 [r6] (raw file):

 * In development, this configuration plays no significant role and serves as a stub.
 */
gulp.task('locales-for-backend:dev', function() {

Is it okay like this? Called on spawn-backend, which is the development gulp task for running the backend. Also had to set cwd: conf.paths.serve above, so that the file gets picked up.
@bryk PTAL

---

Comments from Reviewable

[bryk]

Review status: 18 of 21 files reviewed at latest revision, 5 unresolved discussions.

---

build/serve.js, line 166 [r6] (raw file):

Previously, taimir (Atanas Mirchev) wrote…

Is it okay like this? Called on spawn-backend, which is the development gulp task for running the backend. Also had to set cwd: conf.paths.serve above, so that the file gets picked up.
@bryk PTAL

Looks good :)

---

Comments from Reviewable

[taimir]

@bryk PTAL, this is ready for a final review :)

[bryk]

Thanks a lot! This was long but we're finally here. Last comment, this time for real.

Previously, taimir (Atanas Mirchev) wrote…

@bryk PTAL, this is ready for a final review :)

---

Reviewed 1 of 2 files at r6, 6 of 6 files at r7.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

i18n/messages-ja.xtb, line 4 [r7] (raw file):

<!DOCTYPE translationbundle>
<translationbundle lang="cs">
  <translation id="2869930599919902352" key="MSG_HELLO_WORLD" source="/home/mirchev/go_workspace/src/github.com/kubernetes/dashboard/.tmp/serve/app-dev.js" desc="app name help text in deploy page">Translation in japanese of the app-name text.</translation>

Can you change this to something valid? Or the-same-as-english?

We don't want to have this released :)

---

Comments from Reviewable

[taimir]

Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

i18n/messages-ja.xtb, line 4 [r7] (raw file):

Previously, bryk (Piotr Bryk) wrote…

Can you change this to something valid? Or the-same-as-english?

We don't want to have this released :)

Ok, let me do this quickly. I'll change the name of the variable as well.

---

Comments from Reviewable

[taimir]

Review status: 17 of 21 files reviewed at latest revision, 4 unresolved discussions.

---

i18n/messages-ja.xtb, line 4 [r7] (raw file):

Previously, taimir (Atanas Mirchev) wrote…

Ok, let me do this quickly. I'll change the name of the variable as well.

Done.

---

Comments from Reviewable

[bryk]

Reviewed 4 of 4 files at r8.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[bryk]

:) Will merge soon.

Previously, bryk (Piotr Bryk) wrote…

:lgtm:

Thanks a lot! This was long but we're finally here. Last comment, this time for real.

---

Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable