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

CLN: Removed class in pandas/tests/series/test_timezones.py #32186

Conversation

SaturnFromTitan
Copy link
Contributor

  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff

@SaturnFromTitan
Copy link
Contributor Author

This is similar to #32184. Nothing in the tests changed, I just removed the unused class interface to simplify the file

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

I dont really see the point of removing the class. Assuming we aren't actually using the setup/teardown methods (which should be fixed), then all other thing equal this is not a big deal. is there anything different about this PR?

@jreback jreback added the Testing pandas testing functions or related to the test suite label Feb 23, 2020
@SaturnFromTitan
Copy link
Contributor Author

As we're not using any functionality of the class interface, then why have it? Having the class means...

  1. it's harder to reason about the test: functions are more independent than methods
  2. it forces black to use more line breaks (we reach the line limit earlier because of the extra indentation)
  3. it adds more clutter

I think the class interface is just a left-over from #22550

@jreback
Copy link
Contributor

jreback commented Feb 23, 2020

As we're not using any functionality of the class interface, then why have it? Having the class means...

  1. it's harder to reason about the test: functions are more independent than methods
  2. it forces black to use more line breaks (we reach the line limit earlier because of the extra indentation)
  3. it adds more clutter

I think the class interface is just a left-over from #22550

@SaturnFromTitan I get it, and generally i agree, however i also think we have many of these, so we are talking many PRs; so unless we are splitting out to separate modules I wouldn't change this.

Now I could be wrong on the number, if you want to see how many single file with a class we have then I could change.

@SaturnFromTitan
Copy link
Contributor Author

Gotcha. Indeed, there are 100+ files like that.

Even though the effort to change this is very small, I guess the review effort would be significant and would be better to be invested elsewhere. Closing this then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Testing pandas testing functions or related to the test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants