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

Return Nil if there is no translation for palette_ params #58008

Merged

Conversation

mgc1194
Copy link
Contributor

@mgc1194 mgc1194 commented Apr 13, 2024

In PR #49395 I added a method to localize the parameters in programming expressions.

In that implementation, I assumed the default value returned when the I18n API did not find any localized content, was the same as the original content (an Array of Hashes).

However, after a regression in the content, we analyzed more in-depth what the I18n.t method was returning and found out only the first element of the array was return when defaulting.

palette_params: [{"name"=>"[id]", "type"=>"string", "required"=>true, "description"=>"The unique identifier for the canvas screen element. The id is used for referencing the canvas functions or other UI element modification functions. Must begin with a letter, contain no spaces, and may contain letters, digits, - and _."}, {"name"=>"[width]", "type"=>"number", "description"=>"The horizontal width in pixels of the canvas. If not specified the app window width is used."}, {"name"=>"[height]", "type"=>"number", "description"=>"The vertical height in pixels of the canvas. If not specified the app window height is used."}]

i18n_params: {"name"=>"[id]", "type"=>"string", "required"=>true, "description"=>"The unique identifier for the canvas screen element. The id is used for referencing the canvas functions or other UI element modification functions. Must begin with a letter, contain no spaces, and may contain letters, digits, - and _."}

Instead, I decided to return Nil Value by default (when no localized content is found) and then, when the localized content is not Nil, merge the localized parameters with the non-localized ones.

Localized params look like this: {:"[height]"=>{:type=>"número"}, :"[width]"=>{:type=>"número"}}

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@mgc1194 mgc1194 changed the title rturn nil if there is no translation for palette_ params Return Nil if there is no translation for palette_ params Apr 13, 2024
smart: true
)
if i18n_params != localized_params
unless i18n_params.nil?
Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly right.

Copy link
Contributor

Choose a reason for hiding this comment

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

An alternative would be to have default: {} and then remove the condition altogether.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather not process the content if translations aren't available. We would only iterate over the parameters when there's a translation, avoiding errors such as the one found in honeybadger.

smart: true
)
if i18n_params != localized_params
unless i18n_params.nil?
localized_params&.each do |param|
param_key = param['name'].to_sym
Copy link
Contributor

Choose a reason for hiding this comment

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

You would still need a next unless param.key?('name') here to fix the honeybadger error in the thread... I guess... since we are here already.

Copy link
Contributor Author

@mgc1194 mgc1194 Apr 13, 2024

Choose a reason for hiding this comment

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

My theory is that if there is no name param, the string won't be picked up in the sync, and there won't ever be a translation available, returning nil.

Copy link
Contributor

Choose a reason for hiding this comment

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

it's still going to render incorrectly... they need to fix the content, imo. we should not mitigate it by assuming the inappropriate metadata can happen.

@wilkie
Copy link
Contributor

wilkie commented Apr 13, 2024

This is a good change for the sake of keeping the code logical. Technically, it does not matter too much. The default would always yield some kind of dict that differs from localized_params and then it would just kind of fail to do anything with it leading to it to return the right thing anyway.

But that's obviously very confusing... but nothing that's causing a user-facing issue.

@mgc1194 mgc1194 merged commit cc70a08 into staging Apr 13, 2024
2 checks passed
@mgc1194 mgc1194 deleted the fix-programming_environments-localized-palette_params branch April 13, 2024 02:49
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.

None yet

2 participants