Skip to content

Fix issue #4496: tslib.tz_convert(vals, tz1, tz2) may raise an IndexErro...#4497

Merged
jreback merged 1 commit into
pandas-dev:masterfrom
stenri:tslib_tz_convert_trans_pos_plus_1__bugfix
Aug 23, 2013
Merged

Fix issue #4496: tslib.tz_convert(vals, tz1, tz2) may raise an IndexErro...#4497
jreback merged 1 commit into
pandas-dev:masterfrom
stenri:tslib_tz_convert_trans_pos_plus_1__bugfix

Conversation

@stenri

@stenri stenri commented Aug 7, 2013

Copy link
Copy Markdown
Contributor

Bug-fix for Issue #4496 Bug: tslib.tz_convert(vals, tz1, tz2) may raise an IndexError exception.

@cpcloud

cpcloud commented Aug 7, 2013

Copy link
Copy Markdown
Member

couple of things, please do the following

  • add a test
  • hook up travis CI
  • add documentation to doc/release.rst, preferentially move the docstring in your test to release notes, but not necessary. if you just want to add a blurb that's fine too.

Comment thread pandas/tests/test_frame.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You should rewrite this according to what the hour should be here, don't catch every exception: that could hide another bug.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Fixed.

Should I create a new Pull request, or just wait while this one is approved?

@stenri

stenri commented Aug 7, 2013

Copy link
Copy Markdown
Contributor Author
  • hook up travis CI // done;
  • add documentation to doc/release.rst // done (not sure whether such a long bug description is appropriate in release.rst);
  • "You should rewrite this according to what the hour should be here, don't catch every exception: that could hide another bug." // That's reasonable. Thanks! Fixed.

@jreback

jreback commented Aug 23, 2013

Copy link
Copy Markdown
Contributor

@stenri can you rebase and hook up travis?

@stenri

stenri commented Aug 23, 2013

Copy link
Copy Markdown
Contributor Author

Surely. I rebased tslib_tz_convert_trans_pos_plus_1__bugfix against upstream/master (and combined all my commits into a one commit).

Comment thread doc/source/release.rst

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can you just make this a one-liner here? (and put the explanation with the test, though could be a bit shorter)...thanks

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Well.. ok. Though Phillip Cloud asked me to do something that looks exact the opposite (initially my detailed explanation was located within the test doc string):

"add documentation to doc/release.rst, preferentially move the docstring in your test to release notes, but not necessary. if you just want to add a blurb that's fine too."

Anyway, I'll try to make my explanation shorter, and put a one-liner in release.rst.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

sorry for the confusion. The idea is that release notes is a pretty short explanation, a doc-string is user visibile should shouldn't be that crazy. Docs can be full explanation. But, in this case its actually a bug fix reference (that won't be there after), so can put in the test suite.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I see. I removed detailed explanation from release.rst. Now both release.rst and doc string in unit-test reference Issue #4496, where I wrote the detailed bug description. So interested person can look there for details.

@jreback

jreback commented Aug 23, 2013

Copy link
Copy Markdown
Contributor

@stenri can you squash into 1 commit? then will merge

@stenri

stenri commented Aug 23, 2013

Copy link
Copy Markdown
Contributor Author

Excellent! Done.

jreback added a commit that referenced this pull request Aug 23, 2013
…1__bugfix

Fix issue #4496: tslib.tz_convert(vals, tz1, tz2) may raise an IndexErro...
@jreback jreback merged commit f41e929 into pandas-dev:master Aug 23, 2013
@jreback

jreback commented Aug 23, 2013

Copy link
Copy Markdown
Contributor

thanks!

@stenri stenri deleted the tslib_tz_convert_trans_pos_plus_1__bugfix branch August 23, 2013 14:26
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