Skip to content

Conversation

hewittk
Copy link
Contributor

@hewittk hewittk commented Mar 25, 2021

@pep8speaks
Copy link

pep8speaks commented Mar 25, 2021

Hello @hewittk! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-06-25 03:56:00 UTC

@hewittk hewittk marked this pull request as ready for review March 25, 2021 12:34
@hewittk
Copy link
Contributor Author

hewittk commented Mar 25, 2021

This pull request makes for more consistency and flexibility of the arguments for series.between and the validate_endpoints method of date_timelike.py which is the underlying method of closed for date_range. The parameters that can be inputted to the methods are either a boolean value or a string value of 'both', 'left', 'right', or 'neither'.

@hewittk hewittk changed the title New boundary inputs ENH: New boundary inputs Mar 25, 2021
@mzeitlin11
Copy link
Member

Thanks for the pr @hewittk! Looks like some unrelated changes crept in - seeing some pip related additions which should be reverted

@hewittk
Copy link
Contributor Author

hewittk commented Mar 25, 2021

Thank you for letting me know @mzeitlin11! The pip files should be deleted now, let me know if there are any unrelated additions still there that I haven't noticed.

@mzeitlin11
Copy link
Member

Thank you for letting me know @mzeitlin11! The pip files should be deleted now, let me know if there are any unrelated additions still there that I haven't noticed.

Looks good now. I think codecov is failing - if you added new options to parameters, we need to make sure that they're all tested (and a whatsnew note should be added explaining user-facing changes there are here)

Copy link
Contributor

@attack68 attack68 left a comment

Choose a reason for hiding this comment

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

Thanks @hewittk for the PR. I made some comments inline. Some of the changes minght be considered non-controversial, and others we might some input from pandas core devs.

Copy link
Member

@simonjayhawkins simonjayhawkins left a comment

Choose a reason for hiding this comment

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

Thanks @hewittk docstring validation failure to be resolved before merge, otherwise lgtm

@jreback
Copy link
Contributor

jreback commented Jun 24, 2021

@hewittk can you merge master and fix the doc-string

@jreback
Copy link
Contributor

jreback commented Jun 25, 2021

there is a test failure on this build: https://github.com/pandas-dev/pandas/pull/40628/checks?check_run_id=2909716187
likely need to change the inclusive argument in the test.

@simonjayhawkins simonjayhawkins merged commit eca7655 into pandas-dev:master Jun 25, 2021
meeseeksmachine pushed a commit to meeseeksmachine/pandas that referenced this pull request Jun 25, 2021
@simonjayhawkins
Copy link
Member

Thanks @hewittk

simonjayhawkins added a commit to simonjayhawkins/pandas that referenced this pull request Jun 25, 2021
simonjayhawkins pushed a commit that referenced this pull request Jun 25, 2021
meeseeksmachine pushed a commit to meeseeksmachine/pandas that referenced this pull request Jun 25, 2021
simonjayhawkins added a commit that referenced this pull request Jun 25, 2021
simonjayhawkins added a commit that referenced this pull request Jun 25, 2021
JulianWgs pushed a commit to JulianWgs/pandas that referenced this pull request Jul 3, 2021
JulianWgs pushed a commit to JulianWgs/pandas that referenced this pull request Jul 3, 2021
@jbrockmendel
Copy link
Member

@hewittk want to make a PR enforcing this deprecation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API - Consistency Internal Consistency of API/Behavior Deprecate Functionality to remove in pandas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ENH: consistency of input args for boundaries
9 participants