-
Notifications
You must be signed in to change notification settings - Fork 900
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
Remove noisy test output logging we've never cared about #23213
Conversation
Technically, we could try to fix this and get the translators to update their data to have this fixed, but it's possible there are situations where this happens again and it's not an actual problem for gettext. Even the original PR that added this said this overall number of newlines check could flag intentionally changed values. Since we've never reacted to this output, we can remove it. From that PR (we care about the 3rd check below): 1. if original string starts with a newline, translation needs to start with a newline too 2. if original string ends with a newline, translated string also needs to end with a newline 3. overal amount of newlines in original and translated string needs to match In this PR, I am introducing a new test, which tests the above rules. The exception is the 3rd rule, since it seems that in some places the newline might have been added (or omitted) intentionaly. So we're just logging these. Reference:: ManageIQ#20815
FYI, this is the output we're removing:
|
Checked commit jrafanie@e590ce0 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint |
While I don't like the noise, this test was here for a reason and that's that the translators are incorrectly translating. So, taking this for an example:
That's showing me that the 1) and 2) are inline, so presentation wise it's bad...this is what the test is supposed to show and the goal was to either
I'd really rather not delete this test, because it does have value. Incidentally, if you look closely it's like only 2 strings at fault, so IMO we should just fix them. |
I am curious where these are even being displayed in the UI. newlines are notoriously bad for UI because they don't necessarily render as a |
@Fryguy I see that we have a few possibilities:
Since we haven't done anything for the past 4 years, not sure how much we want to spend on a solution. But the proper solution is number 3. Just not sure how to do this. The least expensive option is 4 - just have to click merge on that one. |
Or my number 2
Which, given there are literally only 3 strings in the list, seems like the simplest option. |
yeah, I don't know. Is it worth the effort to track them down and fix? |
|
My thinking on this PR is YAGNI. It's been a noisy log message since it's introduction. It was never worthy of failing tests so it was never that important to fail tests. We've not cared about it until now so I figured we can add it back if we suddenly want to track these down and fix them. |
This is an easy solve - here are the places that these strings live and none of them actually need newlines, so I'll just make PRs, and then we can leave this test. Also, I was wrong - it was 4 strings, not 3 😛
|
Newlines can cause trouble with translations as well as presentation in the UI. This particular string doesn't actually need the newlines, so this commit changes it to just not have them. Alternative to ManageIQ/manageiq#23213
If this was important why was it not failing the tests? Should I make this test fail and not just muddy the output when running the tests locally or in CI? I didn't want to spend any time on it since the comments in the PR and the code indicate it wasn't actually a problem all the time. But if all of the strings can be fixed and verified as working in the UI so it sounds like we should really fail the test and possibly offer exceptions if there ever is an exception. |
I disagree with the initial assessment from Milan that sometimes it was necessary to change the newlines. In all of these cases, the translators just did it wrong. I agree with you that this test should fail, not warn, and that way we can actually catch these from importing the automated translations at the time they are imported. In fact, adding newlines is a thing we don't want the translators to do cause that can actually break outputs, so this test is really important to find those. It also happens that we don't need the newlines here even in English, so changing them seems like a simple solution as these strings will then just be resubmitted to the translation team. |
I can followup and make it fail once they're fixed.
Yeah, now that we do scheduled runs of po/pot generation and failing this test in the future, we'll be picking any of these up shortly after it's committed.
Right, any solution should be in the english pot so we can get them translated as it's much easier to follow the normal pattern vs. asking them to manually update their existing data to manually update their translations. |
Newlines can cause trouble with translations as well as presentation in the UI. This particular string doesn't actually need the newlines, so this commit changes it to just not have them. Alternative to ManageIQ/manageiq#23213
Newlines can cause trouble with translations as well as presentation in the UI. This particular string doesn't actually need the newline as part of the translation string, so this commit changes it to just not have it. Alternative to ManageIQ#23213
Newlines can cause trouble with translations as well as presentation in the UI. This particular string doesn't actually need the newline as part of the translation string, so this commit changes it to just not have it. Alternative to ManageIQ#23213
Newlines can cause trouble with translations as well as presentation in the UI. This particular string doesn't actually need the newline as part of the translation string, so this commit changes it to just not have it. Alternative to ManageIQ#23213
Newlines can cause trouble with translations as well as presentation in the UI. This particular string doesn't actually need the newlines, so this commit changes it to just not have them. Alternative to ManageIQ/manageiq#23213
I'll open a PR to make this situation raise an error instead of flooding stdout and getting largely ignored. |
Followup to: ManageIQ#23213 We found the overall number of newlines test failures between English strings and their translations was printed to the screen but not failing the tests. This was ignored. It was decided in the above PR that this test was valid and should fail the tests on failure. I also changed the output to be a bit more readable. Before: ``` Randomized with seed 49148 The following translation entries do not honor overall number of newlines: >> File: /Users/joerafaniello/Code/manageiq/locale/es/manageiq.po ---------- »VMs must be scanned from an EVM server whose host is attached to the same storage as the VM unless overridden via SmartProxy affinity. Please verify that: 1) Direct LUNs are attached to ManageIQ appliance 2) Management Relationship is set for the ManageIQ appliance« »Se deben analizar las MV desde un servidor EVM cuyo host esté conectado al mismo almacenamiento que la MV, a menos que se anule mediante afinidad con SmartProxy. Compruebe que: 1) Los LUN directos estén conectados a un dispositivo ManageIQ 2) La relación de gestión esté configurada para el dispositivo ManageIQ« ---------- >> File: /Users/joerafaniello/Code/manageiq/locale/it/manageiq.po ---------- »VMs must be scanned from an EVM server whose host is attached to the same storage as the VM unless overridden via SmartProxy affinity. Please verify that: 1) Direct LUNs are attached to ManageIQ appliance 2) Management Relationship is set for the ManageIQ appliance« »Le VM devono essere scansate da un server EVM il cui host è collegato alla stessa memoria della VM a meno che non sovrascrivi tramite affinità SmartProxy. Si prega di verificare che: 1) I LUN diretti siano collegati al dispositivo ManageIQ 2) La Relazione di gestione sia impostata per il dispositivo ManageIQ« ---------- 2) 为 ManageIQ 设备设置了管理关系« ---------- ... >> File: /Users/joerafaniello/Code/manageiq/locale/zh_TW/manageiq.po ---------- »Provision failed for the following reasons: %{errors}« »供應失敗,原因如下:%{errors}« ---------- »VMs must be scanned from an EVM server whose host is attached to the same storage as the VM unless overridden via SmartProxy affinity. Please verify that: 1) Direct LUNs are attached to ManageIQ appliance 2) Management Relationship is set for the ManageIQ appliance« »除非透過 SmartProxy 親緣性置換,否則必須從其主機連接至與 VM 相同的儲存體的 EVM 伺服器中掃描 VM。 請驗證: 1) 直接 LUN 已連接至 ManageIQ 應用裝置 2) 已為 ManageIQ 應用裝置設定管理關係« ---------- . Finished in 2.9 seconds (files took 2.97 seconds to load) 1 example, 0 failures ``` After: ``` 1) newlines translations honor newlines Failure/Error: expect(err).to be_empty, "Rule: #{rule} was violated in:\n #{err_msg}" Rule: equal number of overall newlines was violated in: File: /Users/joerafaniello/Code/manageiq/locale/es/manageiq.po English: "»VMs must be scanned from an EVM server whose host is attached to the same\n storage as the VM unless overridden via SmartProxy affinity.\n Please verify that:\n 1) Direct LUNs are attached to ManageIQ appliance\n 2) Management Relationship is set for the ManageIQ appliance«" Translation: "»Se deben analizar las MV desde un servidor EVM cuyo host esté conectado al mismo almacenamiento que la MV, a menos que se anule mediante afinidad con SmartProxy.\n Compruebe que:\n 1) Los LUN directos estén conectados a un dispositivo ManageIQ\n 2) La relación de gestión esté configurada para el dispositivo ManageIQ«" File: /Users/joerafaniello/Code/manageiq/locale/it/manageiq.po English: "»VMs must be scanned from an EVM server whose host is attached to the same\n storage as the VM unless overridden via SmartProxy affinity.\n Please verify that:\n 1) Direct LUNs are attached to ManageIQ appliance\n 2) Management Relationship is set for the ManageIQ appliance«" Translation: "»Le VM devono essere scansate da un server EVM il cui host è collegato alla stessa memoria della VM a meno che non sovrascrivi tramite affinità SmartProxy.\n Si prega di verificare che: 1) I LUN diretti siano collegati al dispositivo ManageIQ 2) La Relazione di gestione sia impostata per il dispositivo ManageIQ«" File: /Users/joerafaniello/Code/manageiq/locale/pt_BR/manageiq.po English: "»Enables defining a URL path prefix for XCCDF file instead of accessing the default location.\n example: http://my_file_server.org:3333/xccdf_files/\n Expecting to find com.redhat.rhsa-RHEL7.ds.xml.bz2 file there.«" Translation: "»Permite definir um prefixo de caminho de URL para o arquivo XCCDF em vez de acessar o local padrão.\n Por exemplo: http://my_file_server.org: 3333/xccdf_files/ Esperando encontrar o arquivo com.redhat.rhsa-RHEL7.ds.xml.bz2 lá.«" English: "»Enables defining a URL path prefix for XCCDF file instead of accessing the default location.\nexample: http://my_file_server.org:3333/xccdf_files/\nExpecting to find com.redhat.rhsa-RHEL7.ds.xml.bz2 file there.«" Translation: "»Permite definir um prefixo de caminho de URL para o arquivo XCCDF em vez de acessar o local padrão.\nPor exemplo: http://my_file_server.org: 3333/xccdf_files/ Esperando encontrar o arquivo com.redhat.rhsa-RHEL7.ds.xml.bz2 lá.«" English: "»Provision failed for the following reasons:\n%{errors}«" Translation: "»A provisão falhou pelos seguintes motivos: %{errors}«" English: "»VMs must be scanned from an EVM server whose host is attached to the same\n storage as the VM unless overridden via SmartProxy affinity.\n Please verify that:\n 1) Direct LUNs are attached to ManageIQ appliance\n 2) Management Relationship is set for the ManageIQ appliance«" Translation: "»As VMs devem ser escaneadas a partir de um servidor EVM cujo host esteja conectado ao mesmo armazenamento que a VM, a menos que seja substituído por meio da afinidade do SmartProxy.\n Verifique se: 1) LUNs diretas estão conectadas ao dispositivo ManageIQ\n2) O relacionamento de gerenciamento está configurado para o dispositivo ManageIQ«" File: /Users/joerafaniello/Code/manageiq/locale/zh_CN/manageiq.po English: "»Enables defining a URL path prefix for XCCDF file instead of accessing the default location.\n example: http://my_file_server.org:3333/xccdf_files/\n Expecting to find com.redhat.rhsa-RHEL7.ds.xml.bz2 file there.«" Translation: "»允许为 XCCDF 文件定义 URL 路径前缀,而不是访问缺省位置。示例:http://my_file_server.org:3333/xccdf_files/\n 应该可在那里找到 com.redhat.rhsa-RHEL7.ds.xml.bz2 文件。«" English: "»Enables defining a URL path prefix for XCCDF file instead of accessing the default location.\nexample: http://my_file_server.org:3333/xccdf_files/\nExpecting to find com.redhat.rhsa-RHEL7.ds.xml.bz2 file there.«" Translation: "»允许为 XCCDF 文件定义 URL 路径前缀,而不是访问缺省位置。示例:http://my_file_server.org:3333/xccdf_files/\n 应该可在那里找到 com.redhat.rhsa-RHEL7.ds.xml.bz2 文件。«" English: "»Provision failed for the following reasons:\n%{errors}«" Translation: "»由于以下原因,供应失败:%{errors}«" English: "»VMs must be scanned from an EVM server whose host is attached to the same\n storage as the VM unless overridden via SmartProxy affinity.\n Please verify that:\n 1) Direct LUNs are attached to ManageIQ appliance\n 2) Management Relationship is set for the ManageIQ appliance«" Translation: "»虚拟机必须从 EVM 服务器扫描,它的主机附加到和虚拟机相同的存储器上,除非通过 SmartProxy 关联覆盖。\n 请验证:\n 1) 直接 LUN 附加到 ManageIQ 设备\n 2) 为 ManageIQ 设备设置了管理关系«" File: /Users/joerafaniello/Code/manageiq/locale/zh_TW/manageiq.po English: "»Datastore import was successful.\nNamespaces updated/added: %{namespace_stats}\nClasses updated/added: %{class_stats}\nInstances updated/added: %{instance_stats}\nMethods updated/added: %{method_stats}«" Translation: "»成功匯入資料儲存庫。已更新/已新增名稱空間:%{namespace_stats}\n已更新/已新增類別:%{class_stats}\n已更新/已新增實例:%{instance_stats}\n已更新/已新增方法:%{method_stats}«" English: "»Enables defining a URL path prefix for XCCDF file instead of accessing the default location.\n example: http://my_file_server.org:3333/xccdf_files/\n Expecting to find com.redhat.rhsa-RHEL7.ds.xml.bz2 file there.«" Translation: "»啟用為 XCCDF 檔案定義 URL 路徑字首而非存取預設位置的功能。例如:http://my_file_server.org:3333/xccdf_files/,期望在這裡能夠找到 com.redhat.rhsa-RHEL7.ds.xml.bz2 檔。«" English: "»Enables defining a URL path prefix for XCCDF file instead of accessing the default location.\nexample: http://my_file_server.org:3333/xccdf_files/\nExpecting to find com.redhat.rhsa-RHEL7.ds.xml.bz2 file there.«" Translation: "»啟用為 XCCDF 檔案定義 URL 路徑字首而非存取預設位置的功能。例如:http://my_file_server.org:3333/xccdf_files/,期望在這裡能夠找到 com.redhat.rhsa-RHEL7.ds.xml.bz2 檔。«" English: "»Provision failed for the following reasons:\n%{errors}«" Translation: "»供應失敗,原因如下:%{errors}«" English: "»VMs must be scanned from an EVM server whose host is attached to the same\n storage as the VM unless overridden via SmartProxy affinity.\n Please verify that:\n 1) Direct LUNs are attached to ManageIQ appliance\n 2) Management Relationship is set for the ManageIQ appliance«" Translation: "»除非透過 SmartProxy 親緣性置換,否則必須從其主機連接至與 VM 相同的儲存體的 EVM 伺服器中掃描 VM。\n 請驗證: 1) 直接 LUN 已連接至 ManageIQ 應用裝置 2) 已為 ManageIQ 應用裝置設定管理關係«" Shared Example Group: :newlines called from ./spec/i18n/newlines_spec.rb:2 # ./spec/shared/i18n/newlines.rb:51:in `block (3 levels) in <main>' # ./spec/shared/i18n/newlines.rb:40:in `each' # ./spec/shared/i18n/newlines.rb:40:in `block (2 levels) in <main>' # /Users/joerafaniello/.gem/ruby/3.1.5/gems/webmock-3.24.0/lib/webmock/rspec.rb:39:in `block (2 levels) in <main>' Finished in 3.01 seconds (files took 2.75 seconds to load) 1 example, 1 failure ```
Technically, we could try to fix this and get the translators to update their data to have this fixed, but it's possible there are situations where this happens again and it's not an actual problem for gettext.
Even the original PR that added this said this overall number of newlines check could flag intentionally changed values. Since we've never reacted to this output, we can remove it.
From that PR (we care about the 3rd check below):
Reference:: #20815