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

Active Admin developer can pass options for CSV generation #1626

Merged
merged 3 commits into from
Dec 3, 2012

Conversation

rheaton
Copy link
Contributor

@rheaton rheaton commented Aug 30, 2012

For example, in active admin config: config.csv_options = { :force_quotes => true }
or in the active admin resource definition: csv :options => { :force_quotes => true} do . . .

@travisbot
Copy link

This pull request passes (merged 13e7d97b into 6f0d691).

@jpmckinney
Copy link
Contributor

Hmm, we should maybe revert the recently merged #1608 and otherwise use the solution in this current request, as it is more flexible. Can you do that in this pull request?

@pcreux
Copy link
Contributor

pcreux commented Sep 3, 2012

I'd like to keep the two behaviours (from #1608 and #1626) together until we release 0.6.0. API should not change between patch versions.

@rheaton Could you update the documentation under doc/ for customizing in initializer as well as at the resource level?

@rheaton
Copy link
Contributor Author

rheaton commented Sep 3, 2012

I can update the docs now.

I should not merge in the separator key with the other options, right?

Sorry, I was away helping family all weekend!

@jpmckinney
Copy link
Contributor

I think it makes sense to merge the separator key.

@rheaton
Copy link
Contributor Author

rheaton commented Sep 4, 2012

@pcreux can I get consensus on this before I change it?
Thanks!

@jpmckinney
Copy link
Contributor

I agree with @pcreux that both behaviors should be kept. I think there is consensus.

@@ -7,7 +7,10 @@
CSV
end

csv_output = csv_lib.generate(:col_sep => active_admin_config.csv_builder.column_separator || active_admin_application.csv_column_separator) do |csv|
col_sep = active_admin_config.csv_builder.column_separator || active_admin_application.csv_column_separator
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a deprecation warning here when col_sep is set. It will be removed in ActiveAdmin 0.6.0.

@rheaton
Copy link
Contributor Author

rheaton commented Sep 25, 2012

Yup, will do tomorrow. I'm planning on working on issue #1694 as well. :)

@rheaton
Copy link
Contributor Author

rheaton commented Sep 26, 2012

The deprecation warning is all over the specs, should I try to get rid of this? I noticed there were a few others as well.

@pcreux
Copy link
Contributor

pcreux commented Dec 3, 2012

Hey @rheaton Could you rebase your changes against master so that I can merge this in? :)

For example, in active admin config: `config.csv_options = { :force_quotes => true }`
or in the active admin resource definition: `csv :options => { :force_quotes => true} do . . .`
@rheaton
Copy link
Contributor Author

rheaton commented Dec 3, 2012

Done!

pcreux added a commit that referenced this pull request Dec 3, 2012
Active Admin developer can pass options for CSV generation
@pcreux pcreux merged commit 1eb699d into activeadmin:master Dec 3, 2012
@pcreux
Copy link
Contributor

pcreux commented Dec 3, 2012

🤘

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