Skip to content

Release notes don't warn/inform about skip_footer changes #1948

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

Closed
gerigk opened this issue Sep 22, 2012 · 7 comments
Closed

Release notes don't warn/inform about skip_footer changes #1948

gerigk opened this issue Sep 22, 2012 · 7 comments
Labels
Milestone

Comments

@gerigk
Copy link

gerigk commented Sep 22, 2012

This change ( before, skipping the last row was skip_footer=1, now this is accomplished by skip_footer=-1, otherwise you will only receive one row) should be announced clearly in the release notes imho.

@wesm
Copy link
Member

wesm commented Sep 22, 2012

I wasn't aware of this API change. @changhiskhan?

Sent from my mobile device

On Sep 22, 2012, at 1:06 PM, Arthur Gerigk notifications@github.com wrote:

This change ( before, skipping the last row was skip_footer=1, now this is
accomplished by skip_footer=-1, otherwise you will only receive one row)
should be announced clearly in the release notes imho.


Reply to this email directly or view it on
GitHubhttps://github.com//issues/1948.

@gerigk
Copy link
Author

gerigk commented Sep 22, 2012

skip_footer : int, default 0
Lines at bottom of file to skip. If >0 then indicates the row to start
skipping. If <0 then skips the specified number of rows from the end.

from the current doc string. I can confirm the behaviour since it crashed my tests ;)

@changhiskhan
Copy link
Contributor

@gerigk sorry about that. Think of skip_footer as keep rows 0:N
When parsing Excel sheets, some times you know which row you want to stop parsing at and some times you know how many rows at the end to through away. And we didn't want to add an additional parameter.
We'll make a decision whether to stick with the old API or not but if there is API change it will be in the release notes before 0.9 final.

@wesm
Copy link
Member

wesm commented Sep 24, 2012

One option is to revert the API and instruct users who want 0:N to use the
nrows argument

Sent from my mobile device

On Sep 24, 2012, at 1:35 AM, Chang She notifications@github.com wrote:

@gerigk https://github.com/gerigk sorry about that. Think of skip_footer
as keep rows 0:N
When parsing Excel sheets, some times you know which row you want to stop
parsing at and some times you know how many rows at the end to through
away. And we didn't want to add an additional parameter.
We'll make a decision whether to stick with the old API or not but if there
is API change it will be in the release notes before 0.9 final.


Reply to this email directly or view it on
GitHubhttps://github.com//issues/1948#issuecomment-8807905.

@changhiskhan
Copy link
Contributor

oh duh, that should work fine actually

@gerigk
Copy link
Author

gerigk commented Sep 24, 2012

slightly unrelated: what about unifying skiprows and skip_footer to either
no underscore or both with an underscore?
one could use a deprecation message for transition.

On Mon, Sep 24, 2012 at 2:05 PM, Chang She notifications@github.com wrote:

oh duh, that should work fine actually


Reply to this email directly or view it on GitHubhttps://github.com//issues/1948#issuecomment-8816555.

@changhiskhan
Copy link
Contributor

The API change was reverted. skip_footer is now exposed for ExcelFile.parse as well and skipfooter is aliased to skip_footer now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants