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.

Sign up for GitHub

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

Jump to bottom

BUG: freqstr may be parsed incorrectly #13930

Closed
wants to merge 1 commit into from
Closed

BUG: freqstr may be parsed incorrectly #13930

wants to merge 1 commit into from

Conversation

sinhrks
Copy link
Member

@sinhrks sinhrks commented Aug 8, 2016

  • tests added / passed
  • passes git diff upstream/master | flake8 --diff
  • whatsnew entry

Invalid freq like below doesn'traise ValueError properly. The PR makes all the below raise.

pd.tseries.frequencies.to_offset('U1')
# <Micro>
pd.tseries.frequencies.to_offset('3U1')
# <3 * Micros>
pd.tseries.frequencies.to_offset('-33-22U')
# <-22 * Micros>

@sinhrks sinhrks added Bug Frequency DateOffsets labels Aug 8, 2016
@sinhrks sinhrks added this to the 0.19.0 milestone Aug 8, 2016
@jreback
Copy link
Contributor

jreback commented Aug 8, 2016

might fix / xref to #8419

@codecov-io
Copy link

codecov-io commented Aug 8, 2016 •
edited
Loading

Current coverage is 85.30% (diff: 100%)

Merging #13930 into master will increase coverage by <.01%

@@             master     #13930   diff @@
==========================================
  Files           139        139          
  Lines         50157      50162     +5   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          42785      42791     +6   
+ Misses         7372       7371     -1   
  Partials          0          0

Powered by Codecov. Last update b7abef4...e797a2d

@sinhrks
Copy link
Member Author

sinhrks commented Aug 8, 2016

Now pd.tseries.frequencies.to_offset('.5S') explicitly raises. I'll comment on #8419.

@sinhrks sinhrks mentioned this pull request Aug 8, 2016
Closed
@jreback
Copy link
Contributor

jreback commented Aug 8, 2016

@sinhrks merged #13874 so this needs a rebase

@sinhrks
Copy link
Member Author

sinhrks commented Aug 11, 2016

ok, rebased.

jreback
jreback reviewed Aug 12, 2016
View reviewed changes
pandas/tseries/frequencies.py
splitted = re.split(opattern, freq)
if splitted[-1] != '' and not splitted[-1].isspace():
# the last element must be blank
raise ValueError('includes garbage characters')
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a better message here?

Copy link
Member Author

Choose a reason for hiding this comment

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

this is an internal description which is catched immediately. Users wil see " invalid frequency: xxx" as an error.

@jreback
Copy link
Contributor

jreback commented Aug 12, 2016

minor change. lgtm. ping when green.

@jreback jreback closed this in 7a8d224 Aug 13, 2016
@jreback
Copy link
Contributor

jreback commented Aug 13, 2016

thanks!

@sinhrks sinhrks deleted the freq_invalid branch August 13, 2016 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug Frequency DateOffsets
Projects
None yet
Milestone
0.19.0
Development

Successfully merging this pull request may close these issues.
3 participants
@sinhrks @jreback @codecov-io