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

Encode any values for CSV #3130

Closed
wants to merge 1 commit into from

Conversation

dtaniwaki
Copy link
Contributor

Hi,

Thank you for merging the PR about CSV encoding. I found a bug in an edge case after it was merged.

If a model has columns with serialized object, they don't respond to "encode!" but can include strings to encode.

e.g.
{a: "a", b: "あいうえお"}

["a", "あいうえお"]

So my solution here is encode any values after convert it to string since the values are converted sooner or later in CSV lib.
Could you consider to merge this fix?

@dtaniwaki
Copy link
Contributor Author

@seanlinsley
Do you know why ./script/travis_cache download_bundle failed in travis CI?
I haven't touched anything about this.

@seanlinsley
Copy link
Contributor

No, I don't know why it's a problem on this branch...

@seanlinsley
Copy link
Contributor

The test suite should start working again if you rebase on master.

@dtaniwaki
Copy link
Contributor Author

@seanlinsley
It still fails. The error was S3 credential missing. Do I need do something about the credential on my side?

@seanlinsley
Copy link
Contributor

Sorry, that was my fault. It should be fixed now.

@dtaniwaki
Copy link
Contributor Author

It succeeded now. Thank you!

@dtaniwaki
Copy link
Contributor Author

could you merge this fix??

seanlinsley added a commit that referenced this pull request May 29, 2014
@seanlinsley
Copy link
Contributor

This couldn't be merged, because the CSV code has been changed around a lot lately. However, 3fb4c51 on master should resolve this.

sguignot pushed a commit to sguignot/active_admin that referenced this pull request Oct 28, 2014
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