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

pendulum.DateTime using hardcoded pendulum.datetime in methods #203

Closed
MrGreenTea opened this issue May 9, 2018 · 2 comments · Fixed by #732
Closed

pendulum.DateTime using hardcoded pendulum.datetime in methods #203

MrGreenTea opened this issue May 9, 2018 · 2 comments · Fixed by #732

Comments

@MrGreenTea
Copy link

I found 4 methods in pendulum.datetime that return a new pendulum.datetime object. The pendulum.datetime class is hardcoded in these, which could cause some inconstistencies when subclassing:

import pendulum

class MyDateTime(pendulum.DateTime):
    pass

MyDateTime.now()

The output is an instance of pendulum.DateTime while I would expect it to be an instance of MyDateTime.

This makes extending the library a bit of a hassle.

@MrGreenTea MrGreenTea changed the title pendulum.datetime using hardcoded class in methods pendulum.DateTime using hardcoded pendulum.datetime in methods May 9, 2018
@Delgan
Copy link
Contributor

Delgan commented May 18, 2018

Hey @sdispater, I think I can handle this if you wish.

What made you move the implementations from DateTime to __init__.py in v2?

The issue could be fixed by moving the functions back to DateTime (as private methods if there are not part of the public API). Then use now = DateTime.now in __init__.py like in v1.

Do you have any other plan?

@DanCardin
Copy link

for what it's worth, I ran into this as well, trying to make our code compatible with 2.x. There are similar problems with the parse method, because the parser will just call directly out to pendulum.datetime et al.

I'm also struggling to understand what's going on with __new__, the constructor, and generally the original intent to have all datetimes be timezone aware by default.

I think its effectively infeasible for us to upgrade, unfortunately.

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