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

Add ability to ignore csv fields as requested in #15 #30

Closed
wants to merge 13 commits into from
Closed

Add ability to ignore csv fields as requested in #15 #30

wants to merge 13 commits into from

Conversation

ckirby
Copy link
Contributor

@ckirby ckirby commented Oct 10, 2016

No description provided.

@coveralls
Copy link

coveralls commented Oct 10, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 03970f3 on ckirby:master into 79f71ef on california-civic-data-coalition:master.

@palewire
Copy link
Owner

Thanks for this patch. I'll take a look when I find a minute. Out of curiosity, what is your use case for this feature?

@ckirby
Copy link
Contributor Author

ckirby commented Oct 11, 2016

First let me say that I really like the project and like supporting it with
new code (I also submitted the patch for the static_mapping functionality).
I saw someone submitted issue 15 and thought I could use the feature too.

Use case: I have a system where data providers send us large data sets that
we use postgre_copy to insert. For various reasons we don't update the
model until all providers are onto the new data spec, so being able to
ignore the new fields until flipping the switch on the model is useful.

I also have some thoughts on how to tackle issue #26 (
#26)
would you be interested in a patch with that functionality?

Best,
Chaim

On Mon, Oct 10, 2016 at 9:12 PM, Ben Welsh notifications@github.com wrote:

Thanks for this patch. I'll take a look when I find a minute. Out of
curiosity, what is your use case for this feature?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#30 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAFeHjbzVNBbBK7gv3B7yTigZlJNLJQ_ks5qyoABgaJpZM4KSlQi
.

@coveralls
Copy link

coveralls commented Oct 25, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 73dc5f3 on ckirby:master into 79f71ef on california-civic-data-coalition:master.

palewire added a commit that referenced this pull request Oct 30, 2016
… from the model and the CopyMapping map dict. This is an alternative solution to @ckirby's pull request #30 that does not require adding a new kwarg but instead tries to have the code 'just work.'
@palewire
Copy link
Owner

palewire commented Oct 30, 2016

Moments ago I pushed a change to the master branch that takes a different approach to solving the problem.

Rather than add a new keyword argument that requires users to explicitly list the headers they'd like to skip, I've tried to rework the application so it will "just work" and allow for headers to be omitted from the map input without any extra configuration.

I've added the tests you submitted here to see if I got it right, and they appear to be passing. If you have the time, let me know what you think of this stab at a solution.

@palewire
Copy link
Owner

@ckirby, if you don't protest, I think I might close this pull request and keep the alternative implementation of your feature described above.

@palewire palewire closed this Nov 11, 2016
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.

3 participants