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

Make StripeResponse a new-style class #495

Merged
merged 2 commits into from
Nov 22, 2018
Merged

Make StripeResponse a new-style class #495

merged 2 commits into from
Nov 22, 2018

Conversation

ob-stripe
Copy link
Contributor

r? @remi-stripe
cc @stripe/api-libraries

Make StripeResponse a new-style class. This ensures that the @property decorators work as expected with Python 2.7. (Python 3 doesn't have old-style classes at all so isn't affected.)

@remi-stripe remi-stripe removed their assignment Nov 21, 2018
@remi-stripe
Copy link
Contributor

@ob-stripe Can you assign someone else? I don't really understand that change and the side effects and since tests are broken on top of it (though likely unrelated) and the entire pipfile.lock file changed, I'd rather not blindly approve

@ob-stripe
Copy link
Contributor Author

For context:

  • failing tests are unrelated -- the tests fail on master too. I think it's due to some weird interaction between the latest version of pip or pipenv and Travis' virtualenv
  • updating Pipfile.lock was just a failed attempt at making tests pass
  • there is basically no reason to use old-style classes, ever
  • I tried looking for a way to enforce this via the linter, but it doesn't seem to be possible without writing a custom extension. I assume this is because old-style classes are gone in Python 3.

r? @brandur-stripe

@brandur-stripe
Copy link
Contributor

LGTM.

A little extra information on new and old style classes here. My interpretation as well that it's just strictly better to use new-style at this point.

Too bad about that build. I didn't look into it, but LMK if you want a second pair of eyes on it or anything.

@ob-stripe
Copy link
Contributor Author

I've tested this locally and confirmed that tests pass on all Python versions, so I'm going to go ahead and merge, and try to fix the Travis issue separately.

@ob-stripe ob-stripe merged commit 8523814 into master Nov 22, 2018
@ob-stripe ob-stripe deleted the ob-fix-lgtm-alerts branch November 22, 2018 14:25
@ob-stripe
Copy link
Contributor Author

Released as 2.12.1.

@ob-stripe
Copy link
Contributor Author

Closing the loop re. Travis builds: I've confirmed this is an issue with the latest pipenv. They've already fixed the issue so our builds should start working again as soon as the new version is released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants