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.

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

TST: Split test_offsets.py #30194

Closed
wants to merge 13 commits into from
Closed

Conversation

@Raalsky
Copy link

Raalsky commented Dec 10, 2019

  • closes #27085
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry
Copy link
Member

simonjayhawkins left a comment

Thanks @Raalsky for the PR. making changes and splitting files in the same PR make review more difficult. Is is possible to do just the split or just the fixturisation in this PR and then a follow-on?



@pytest.fixture(params=[getattr(offsets, o) for o in BUSINESS_OFFSETS])
def business_offset_types(request):
"""
Fixture for all the datetime offsets available for a time series.

This comment has been minimized.

Copy link
@simonjayhawkins

simonjayhawkins Dec 11, 2019

Member

same docstring for business_offset_types as date_offset_types docstring. should differentiate.

This comment has been minimized.

Copy link
@Raalsky

Raalsky Jan 15, 2020

Author

Added in #31034

if issubclass(getattr(offsets, o), offsets.MonthOffset) and o != "MonthOffset"
]
)
def month_classes(request):
def business_month_classes(request):
"""
Fixture for month based datetime offsets available for a time series.

This comment has been minimized.

Copy link
@simonjayhawkins

simonjayhawkins Dec 11, 2019

Member

same docstring for business_month_classes as date_month_classes. should differentiate.

This comment has been minimized.

Copy link
@Raalsky

Raalsky Jan 15, 2020

Author

Added in #31034

Copy link
Contributor

jreback left a comment

yeah if you can do a straight move first, then we can fixturize (or the other way around is ok too). having multiple targeted PRs is likely to make review / merge much faster.

@jbrockmendel

This comment has been minimized.

Copy link
Member

jbrockmendel commented Dec 21, 2019

@Raalsky can you rebase

@jreback

This comment has been minimized.

Copy link
Contributor

jreback commented Jan 1, 2020

closing as stale. @Raalsky would love a PR that first moves, then followups to split / parameterize this. make small targeted PRs; we can then merge quickly.

@jreback jreback closed this Jan 1, 2020
@Raalsky

This comment has been minimized.

Copy link
Author

Raalsky commented Jan 15, 2020

Sorry for being off during the holidays. I've prepared the first step (move) test_offsets to test_date_offsets #31031 Will be following by adding fixtures and then business/date split.

@Raalsky

This comment has been minimized.

Copy link
Author

Raalsky commented Jan 15, 2020

Fixtures added in #31034

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.