i18n project

[Sean Colsen]

Rajat,

I noticed that you opened a draft PR today for your i18n work. What’s the difference between that PR and your other draft PR?

Wow your latest PR is enormous. I’m happy to see that you seem to be making significant progress on the project! But I’d like you to break this into smaller pieces. It looks like you’re aiming to complete the entire project in one PR. I was expecting to review your work much earlier in the process. For example, I’d rather first see a PR which installs the requisite libraries and then wraps one very trivial string with translation. That would be the PR where we’d work out the basic patterns and the async loading and such. It would be like a hello world of i18n. Then we’d have a separate PR which translates one piece of rich text. Then a separate PR which begins to translate some of the backend stuff. (And: I’d actually do that last, since I think the backend i18n work is much lower priority.) We’d have a separate PR for the persistence of the user’s preferred language, and so on and so forth. You seem to be running wild with wrapping strings in translation functions all over the codebase, without having first merged some of the more foundational work. At some point that “run wild” phase is certainly appropriate because there are a lot of strings to wrap. But I’d like to lay the groundwork first. The linting rule is another thing I’d like to have figured out before we run wild. Before we run wild we should have all of our ducks neatly lined up.

In general, I think it’s fine for PRs to be quite “deep” when it’s otherwise difficult to avoid. For example, my form system PR was “deep” in that it added many new inter-related abstractions which required substantial review. As well, I think it’s also okay for PRs to be quite “broad” in certain cases. For example, my code formatting PR was “broad” because it touched basically every front end file. Due to that breadth, I spent almost two months chasing merge conflicts with that PR, but it wasn’t a big deal because it didn’t actually required much review. I’d like to avoid PRs with depth and breadth because the combination of lengthy review and recurrent merge conflict resolution can make the process inefficient.

If it would be easy for you to break your existing PR into several pieces, I would really appreciate it. But if that’s too hard, it’s okay. Mostly I just want to make sure you submit something for review as soon as possible so that the problem doesn’t get worse.

​

[Pavish Kumar Ramani Gopal]

Rajat,

I notice that the draft PR is massive. I agree with Sean's suggestions.

Based on our past experiences, everytime there's a massive PR that's broad, the conflict resolution has led to a number of regressions that went unnoticed until a much later period of time.

I'd strongly suggest that this PR be split into a number of smaller PRs to avoid having to go through that again.

My suggestion would be to remove all route and component level translations from this PR (i.e. anything that looks like this: ), and raise them separately, one PR for each top level route and system, and one for the components.

[Pavish Kumar Ramani Gopal]

 My suggestion would be to remove all route and component level translations from this PR (i.e. anything that looks like this: )

i.e. anything that looks like this: `$LL.general.unknownColumn()`, `$LL.recordWidgets.relatedRecord()` etc.,

[Rajat Vijay]

Okay. Let me try & do that.

For the rest of the things that Sean mentioned -

1. I have added a comment here to make the review process easier. It basically divides and points to the file so that smaller foundational parts can be reviewed. FYI @Mukesh Murali (since I review for BE changes from you.)

2. "The linting rule is another thing I’d like to have figured out before we run wild", this is something IMHO should come with the docs for the other devs(including the core team) to add translations and should only be merged once all of the strings are i18n enabled. Since this will start red-lining all of the un-translated strings in the codebase at once.

[Sean Colsen]

Rajat,

I’m suggesting we implement the linting rule before wrapping all our strings because it will allow us to stress-test the linting rule. This is the kind of linting rule that will inevitably produce false positives and false negatives. Observing the performance of the rule alongside our refactoring work will allow us to make adjustments to the rule as we go, thereby minimizing false positives and false negatives in the future.

Of course we wouldn’t want to merge a PR which enables the rule before all strings are wrapped, because that PR wouldn’t pass CI. One option would be to implement the rule, but leave it disabled. This would allow us to incrementally wrap our strings. But the option I’d lean towards is to try putting all the string-wrapping work into one PR, but do that after all the more sophisticated i18n infrastructural groundwork is laid throughout the codebase. Then we actually could put it one PR. The PR would be quite broad, but it would be very simple and easy to review. Once all the groundwork is laid we could even collaborate on a broad PR like that by assigning mutually-exclusive areas of the codebase for each of us to work on, just so we can focus all our efforts on the grunt work at once (and avoid merge conflicts).

​

[Rajat Vijay]

Moved the conversation regarding the ESLint rule here: https://groups.google.com/a/mathesar.org/g/mathesar-developers/c/2_UVa3CKFtY