Fix for #510 self.skiprows

[user32000]

Fix for #510 self.skiprows

[necaris]

👍

[llllllllll]

Thanks for getting a fix written so quickly! This looks great. To help ensure that we don't accidently break this again in the future could you add a test case?

The tests for the csv module are in odo/backends/tests/test_csv.py. A good example for a test would be something like test_pandas_read(). The imporant testing utility for csvs is with filetext('<csv-content>') as fn: which creates a temporary csv populated with some string. This will let you create an example case that shows off skipping some number of rows.

One side comment: do you think this should fail if you pass both header and skiprows?

[user32000]

I’ll give it a go. Thanks for your help

…

On 7 Dec 2016, at 19:09, Joe Jevnik ***@***.***> wrote: Thanks for getting a fix written so quickly! This looks great. To help ensure that we don't accidently break this again in the future could you add a test case? The tests for the csv module are in odo/backends/tests/test_csv.py. A good example for a test would be something like test_pandas_read(). The imporant testing utility for csvs is with filetext('<csv-content>') as fn: which creates a temporary csv populated with some string. This will let you create an example case that shows off skipping some number of rows. One side comment: do you think this should fail if you pass both header and skiprows? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#511 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AP9i7v4xB0OB7BUgBN6Y3n886geXhx9lks5rFwSHgaJpZM4LGb6h>.

[llllllllll]

How's the test coming along? If you don't think you can get to it I can add a test and we can merge this.

[user32000]

Hi Joe! Sorry, got side-tracked. Let me take a look at what’s needed and get back to you

…

On 16 Dec 2016, at 19:58, Joe Jevnik ***@***.***> wrote: How's the test coming along? If you don't think you can get to it I can add a test and we can merge this. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#511 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AP9i7oshKW6FyRGJqWXv1LlRvxl6Eyq0ks5rIu1bgaJpZM4LGb6h>.

[user32000]

Joe,I promise I’m still going to get to this, thanks for your patience!MichaelOn 16 Dec 2016, at 19:58, Joe Jevnik <notifications@github.com> wrote:How's the test coming along? If you don't think you can get to it I can add a test and we can merge this.—You are receiving this because you authored the thread.Reply to this email directly, view it on GitHub, or mute the thread. {"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/blaze/odo","title":"blaze/odo","subtitle":"GitHub repository","main_image_url":"<a href="https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png" class="">https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png</a>","avatar_image_url":"<a href="https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png" class="">https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png</a>","action":{"name":"Open in GitHub","url":"<a href="https://github.com/blaze/odo" class="">https://github.com/blaze/odo</a>"}},"updates":{"snippets":[{"icon":"PERSON","message":"@llllllllll in #511: How's the test coming along? If you don't think you can get to it I can add a test and we can merge this."}],"action":{"name":"View Pull Request","url":"<a href="#511 (comment)" class="">#511 (comment)</a>"}}}

[user32000]

Am still at it… slow going since I’m new to odo!

…

On 20 Dec 2016, at 10:21, Michael Minzlaff ***@***.***> wrote: Joe, I promise I’m still going to get to this, thanks for your patience! Michael > On 16 Dec 2016, at 19:58, Joe Jevnik ***@***.*** ***@***.***>> wrote: > > How's the test coming along? If you don't think you can get to it I can add a test and we can merge this. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub <#511 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AP9i7oshKW6FyRGJqWXv1LlRvxl6Eyq0ks5rIu1bgaJpZM4LGb6h>. >

[llllllllll]

Thanks for working through this! If you have any questions as you go let me know, I may be able to help unblock you.

[user32000]

Hi Joe, OK, having had another look at this, I find myself thinking that the tests in test_sqlite.py seem to capture pretty much what I was trying to do. As to your question (what if both skiprows and header are set and contradict each other), we could filter as follows: if (header is not None) and (skiprows <> 0) and (int(header) <> skiprows) raise ValueError(“Both skiprows and header set explicitly, and values contradict each other”) But I didn’t spot any other instances of that level of sanity checking? Let me know what you think. Michael

…

On 20 Dec 2016, at 21:17, Joe Jevnik ***@***.***> wrote: Thanks for working through this! If you have any questions as you go let me know, I may be able to help unblock you. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#511 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AP9i7nGDYEMygNbwZDLkBksmKQGpl89Vks5rKEXTgaJpZM4LGb6h>.

[user32000]

... and as a PS I would still like to understand why the existing tests didn't catch the bug I found. It bothers me that they didn't. So maybe some additional test is required after all? Michael

…

On 20 Dec 2016, at 21:17, Joe Jevnik ***@***.***> wrote: Thanks for working through this! If you have any questions as you go let me know, I may be able to help unblock you. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.