Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

[GAPRINDASHVILI] Update gettext catalogs for gaprindashvili update #440

Conversation

mzazrivec
Copy link
Contributor

@mzazrivec mzazrivec commented Jun 26, 2018

  • Updated .pot file
  • zanata.xml for gaprindashvili branch
  • Translations for gaprindashvili update release

@AparnaKarve
Copy link
Contributor

@mzazrivec It looks like not all __( strings were captured here.

For e.g, Check out the line below - https://github.com/ManageIQ/miq_v2v_ui_plugin/blob/master/app/javascript/react/screens/App/Overview/components/InfrastructureMappingsList/InfrastructureMappingsList.js#L122

Does the gettext rake task need __("Text") to be in a single line?
If so, we have 2 options here -

  • Fix the rake task to take into account the above case
  • Relax the rule error Insert ⏎········· prettier/prettier while running npm test

@priley86 Is it possible to remove the above prettier rule for npm?

@mzazrivec
Copy link
Contributor Author

Yes, having {__("Whatever")} on single line would resolve this issue.

@AparnaKarve
Copy link
Contributor

What is interesting is that substitution seems to work when gettext is spread over 1+ lines

{__(
"Whatever"
)}

For the above case, Substitution works
... Extraction does not

Would be nice to keep it consistent for Substitution & Extraction.

error Insert⏎·········prettier/prettier is a generic rule applicable for the overall code, not sure if we can make an exception to this rule for gettext.
@priley86 What do you think? can we?

Copy link
Member

@AllenBW AllenBW left a comment

Choose a reason for hiding this comment

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

Jumped back, looked around 👍 (aside from missing translations)

Copy link
Contributor

@AparnaKarve AparnaKarve left a comment

Choose a reason for hiding this comment

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

Well, a few translations are often missed due to human error factors like forgetting to add gettext.

@AllenBW This case however, is different... here we know that there is an issue. (and it's hard to ignore that)
The least we could do is at least discuss on how to resolve this.

I'm OK to merge this assuming there is a follow-up to fix the remainder strings either via the rake task fix or a fix from our end by ignoring the npm linter rules

@AllenBW
Copy link
Member

AllenBW commented Jun 26, 2018

@AparnaKarve yeah! all for discussing, apologies, my intention wasn't to seemingly ignore the issue 😆

When yah present the options that way, I'm in favor of fixing the rake task (which wouldn't be a thing for this repo) rather than linting rules, which would have far greater implications.

Edit: furthermore, DANG you just wrote the thought on my mind... that this is applicable to other associated code ❤️

@AparnaKarve
Copy link
Contributor

@AllenBW No worries :) Thanks.

Agree that this should be fixed by the rake task -- since it could be applicable to other JS repos/sources too

I'm not sure on the level of effort required to fix the rake task, but if it's not doable for the upcoming G release, then we just need to make a note of it here before merging this and fix it later.

@AllenBW
Copy link
Member

AllenBW commented Jun 26, 2018

Looks like some action happens here 🤔

@skateman @martinpovolny ☝️ any thoughts on feasibility of extracting text across lines?

@priley86
Copy link
Member

priley86 commented Jun 26, 2018

@AparnaKarve @AllenBW yea - it would be ideal to not have to change this. Prettier is an opinionated formatter and unfortunately I don't know a simple fix other than ignoring the file w/ .prettierignore. My preference is to maintain the default Prettier formatting.

EDIT: it appears there is a // prettier-ignore line syntax now... that's news to me...
https://prettier.io/docs/en/ignore.html#javascript

Should just be used w/ caution.

@himdel
Copy link
Contributor

himdel commented Jun 27, 2018

So....we have to write each string to use 3 lines instead of one, and prepare tooling just to support syntax that's not used elsewhere.

Or, we can drop prettier, create a compatible eslint ruleset, and add a rule to support the strings as they should be.

At least that's how I see it :)

@mzazrivec
Copy link
Contributor Author

Here's the situation.

We cannot easily fix this whole problem with a rake task fix, not on our (ManageIQ) end anyway.

The parser which does the .js parsing here comes from one of the supporting gems, which we do
not control. What the parser does is it goes line by line in the javascript source file and on each line it matches gettext invocations with a regular expression. What this means is that if the gettext invocation is spread over several lines, the parser won't find it.

We could try to fix / extend the parser so that it works with our source, but the way things are, this would be an invasive & complicated change and quite a long shot.

At this moment, I need to give our translators a complete V2V catalog to be translated into several languages for upcoming Gaprindashvili update release. Because of the problem described above, we're currently lacking approximately 300 words, which is quite a bit. So we need to find some solution quite fast here.

I'm leaning towards the proposal suggested by @himdel above: use eslint and relax the styling rules.

@AllenBW
Copy link
Member

AllenBW commented Jun 27, 2018

@himdel when yah put it that way 😬:godmode: naaaaahhhhh rewriting all strings to have 3 lines is no bueno...

I guesssss the question is does miq manage its own getext parser? (if so where?) I was hoping it would be a simple regex rewrite to capture all the content contained within the leading __( till the trailing )

oh well @mzazrivec kinda answered that ❤️

Edit: thinking ignoring prettier is gonna be a bit less painful than yanking it... looking at this presently...

@AparnaKarve
Copy link
Contributor

The parser which does the .js parsing here comes from one of the supporting gems, which we do
not control. What the parser does is it goes line by line in the javascript source file and on each line it matches gettext invocations with a regular expression. What this means is that if the gettext invocation is spread over several lines, the parser won't find it.

Sad...I would have expected better from the gem.
By any chance is an upgrade available for that gem which could have addressed this use case? (wishful thinking)
or alternatively are there other gettext gems geared more towards modern JS code?

@martinpovolny
Copy link
Member

martinpovolny commented Jun 28, 2018

@skateman @martinpovolny point_up any thoughts on feasibility of extracting text across lines?

No idea. On the Ruby side we use a parser. Here it's a regexp which, erhmh, well sucks.

I'm leaning towards the proposal suggested by @himdel above: use eslint and relax the styling rules.

Yes, that's what I'd do. The styling rules are meant to help us. If they don't help then don't use them. The goal is not to honor the rules the goal is to get the job done and have readable code.

@mzazrivec mzazrivec force-pushed the update_gettext_catalogs_for_gaprindashvili_update branch from 234233c to 47d5e24 Compare July 9, 2018 13:39
@mzazrivec
Copy link
Contributor Author

This PR should be good to go.

@AparnaKarve
Copy link
Contributor

Thanks for the update @mzazrivec

It looks like there would be a few more strings that would have to be included in the gettext catalog.

This PR #476 contains strings from the .yaml files from the manageiq-content repo.
(The manageiq-content repo does not seem to be doing any gettext extraction at the moment)

Other than the above PR, there are a couple other unmerged PRs that would be introducing new strings in the UI soon.

So it looks like another round of extraction would be necessary in about a week's time or so.
Just wanted to give you an idea.

Sorry for the trouble and thanks in advance.

@mzazrivec
Copy link
Contributor Author

So it looks like another round of extraction would be necessary in about a week's time or so.
Just wanted to give you an idea.

This PR needs to be tested & merged as it is. It contains the source catalogs (.pot) as well as translations into several languages (.po files). Whether a new translation cycle needs to happen is really a different discussion.

@AparnaKarve
Copy link
Contributor

Ok, if that is the purpose of the PR, then LGTM

@simaishi simaishi merged commit 42c4c26 into ManageIQ:gaprindashvili Jul 11, 2018
@mzazrivec mzazrivec deleted the update_gettext_catalogs_for_gaprindashvili_update branch July 12, 2018 06:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants