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

Fixed intl parsing bug when trying to parse non-string entries #1153

Merged
merged 5 commits into from
Oct 11, 2018

Conversation

sepehr500
Copy link
Contributor

@sepehr500 sepehr500 commented Oct 10, 2018

Fixed intl parsing bug when trying to parse non-string entries.


This change is Reviewable

@sepehr500 sepehr500 changed the title Fixed int parsing bug when trying to parse blank entries Fixed intl parsing failure when trying to parse blank entries Oct 10, 2018
@sepehr500 sepehr500 changed the title Fixed intl parsing failure when trying to parse blank entries Fixed intl parsing bug when trying to parse blank entries Oct 10, 2018
@coveralls
Copy link

coveralls commented Oct 10, 2018

Coverage Status

Coverage remained the same at ?% when pulling c5fd709 on sepehr500:master into d484f8f on shakacode:master.

@justin808
Copy link
Member

@sepehr500 please consult with @vcarel regarding #1129.

After that is done, I can assess this.

@@ -107,7 +107,7 @@ def flatten(translations)
translations.each_with_object({}) do |(k, v), h|
if v.is_a? Hash
flatten(v).map { |hk, hv| h["#{k}.#{hk}".to_sym] = hv }
elsif !v.is_a? Array # Arrays are not supported by react-intl
elsif v.is_a? String # Arrays are not supported by react-intl
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment does not seem appropriate anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vcarel Do you think we should support int and bools as well as strings?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like int and bools are not supported -> https://formatjs.io/guides/message-syntax/

Copy link
Contributor

Choose a reason for hiding this comment

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

...I mean by react-intl.
I don't know, see my comment to @justin808 below. I don't feel this is going in the right direction, although my last PR is part of the problem.
The decision is up to both of you.

@vcarel
Copy link
Contributor

vcarel commented Oct 10, 2018

@sepehr500 What issue does it address? I confess I'm concerned about not exporting empty keys, because react-intl would then output the name of the missing keys instead of just blank strings.

@justin808 The root cause of those issues is that we attempt to pass translations to react-intl which will never be used on React's end. Instead, shouldn't we encouraging users to cleanup their translations files and pass only what React will really use?

@sepehr500
Copy link
Contributor Author

@vcarel There are people who use part of their yml for react and the other part for rails. So forcing people to clean up their yml will not work because the libraries support different data types. This PR is to stop the build from failing when you have a type that can not have .gsub called on it.

@sepehr500 sepehr500 changed the title Fixed intl parsing bug when trying to parse blank entries Fixed intl parsing bug when trying to parse non-string entries Oct 10, 2018
@justin808 justin808 merged commit 6359a6d into shakacode:master Oct 11, 2018
@justin808
Copy link
Member

Nice job @sepehr500. @vcarel please give this a try asap. I'm pushing 11.0.7 ASAP.

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.

4 participants