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

The TranslatedString#to_s now returns nil some cases #136

Closed
raxoft opened this issue Mar 2, 2016 · 9 comments
Closed

The TranslatedString#to_s now returns nil some cases #136

raxoft opened this issue Mar 2, 2016 · 9 comments

Comments

@raxoft
Copy link
Contributor

raxoft commented Mar 2, 2016

The commit 2e54394 introduced some bugs.

First of all, I believe TranslatedString#to_s should check if the @value responds to html_safe, not self.

More important thing however is that now to_s doesn't work at all in some cases. Before, code like this was working fine for translated strings:

puts text.split("\n").first.to_s.encoding

Now it fails (with undefined method encoding for nil:NilClass error). The reason is that split creates array of TranslatedStrings, none of which however have the @value set. Cloning the class of the actual string itself seems to be a feature of split, but it obviously causes mismatch between the value of the string itself and the @value variable, which is now lacking (not to mention the lack of other instance variables, but it's not much of an problem for me).

I believe other String operations might suffer from the same problem, so it might be best to drop the @value idea entirely and base the value of the string solely on the string itself.

@raxoft raxoft changed the title The TranslatedString#to_s now returns nil some cases The TranslatedString#to_s now returns nil some cases Mar 2, 2016
@raxoft
Copy link
Contributor Author

raxoft commented Mar 2, 2016

BTW, the split behaviour can be easily tested with this snippet:

class C < String
end

p C.new("test").split("t").map{ |x| [x.class, x] }

Other string methods really seem to share this class cloning behaviour, such as scan.

@raxoft
Copy link
Contributor Author

raxoft commented Mar 2, 2016

BTW, testing the mentioned commit a bit further, it also seems that the conversion of value to string added to the beginning of TranslatedString#initialize handles only TranslatedString, but not Untranslated strings. As a consequence, calling to_s on Untranslated string now returns TranslatedString, rather than String as expected.

@raxoft
Copy link
Contributor Author

raxoft commented Mar 2, 2016

And while we are at it, the change at translation.rb:96 from @filters to @filter looks just like a typo - there is no @filter instance variable in the Translation class.

@ai
Copy link
Member

ai commented Mar 2, 2016

Thanks for really good investigation. I will look closer on this week.

@raxoft
Copy link
Contributor Author

raxoft commented May 16, 2017

Hello, sorry for the bump, but is there any chance to get this fixed? Alternatively, I can try to create a patch myself. Just one question - do you remember what was the idea behind introducing @value instead of using self as before? Thanks.

@ai
Copy link
Member

ai commented May 16, 2017

Yeap, PR will be very welcome

@raxoft
Copy link
Contributor Author

raxoft commented May 16, 2017

OK, I will have a look. Is support for 2.2+ enough or shall I aim for 2.1 as well?

@ai
Copy link
Member

ai commented May 16, 2017

2.1 support will be good, because I need a major release to drop 2.1 support.

@raxoft
Copy link
Contributor Author

raxoft commented May 17, 2017

OK, the PR is ready in #147.

@ai ai closed this as completed in 5268bc8 May 20, 2017
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

No branches or pull requests

2 participants