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

Use a class method to declare a CSV task #317

Merged
merged 1 commit into from
Feb 1, 2021

Conversation

etiennebarrie
Copy link
Member

In #302 we went with a new subclass, which we refactored to a module to be included in a Task.

We got feedback in #302 (comment) but we can't really change the behavior only based on data from the Run (a CSV file being present), since we need to know something about the class to show the input which results in a CSV file being updated. The class itself needs to somehow declare that it supports CSV.

Rafael pointed out that we shouldn't use include as an API for changing behavior to the classes, we discussed that on Slack but we missed the mark with #313 when we went instead with a superclass, as explained in #310 (comment).

So instead this PR uses a class method on Task, doesn't require the Task authors to include a module or inherit from a different class. They simply call csv_collection. I'm open to feedback about the name, but the idea with this one is that it defines a collection like a Task author would do for you, so there some parallel between defining a method collection and calling csv_collection on the class.

Using a class method opens up features like being able to choose whether the CSV should have headers, changing the column separator, using converters, etc. as arguments to the method call that could be passed in to CSV.new.

Internally this still includes a module into the class rather than adding conditionals to collection and count and adding the csv_content attribute to the receiver.

app/tasks/maintenance_tasks/csv_task.rb Outdated Show resolved Hide resolved
app/tasks/maintenance_tasks/csv_task.rb Outdated Show resolved Hide resolved
@etiennebarrie
Copy link
Member Author

One last thing I'm not sure about is whether to make the method private or not. On one hand it doesn't make sense to be called from outside the class, on the other hand things like attr_reader/belongs_to/before_action are not private and it's fine.

@adrianna-chang-shopify
Copy link
Contributor

One last thing I'm not sure about is whether to make the method private or not.

I would be inclined to keep it public, given that most class-level macros are given public visibility in Rails.

@etiennebarrie etiennebarrie merged commit 5c3a22d into main Feb 1, 2021
@etiennebarrie etiennebarrie deleted the csv-task-as-a-class-method branch February 1, 2021 17:55
@adrianna-chang-shopify adrianna-chang-shopify added the CSV Requirements for CSV feature label Feb 11, 2021
@adrianna-chang-shopify adrianna-chang-shopify temporarily deployed to rubygems February 11, 2021 21:05 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSV Requirements for CSV feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants