slashmili / python-jalali

Jalali calendar binding for Python based on Python's datetime module
http://pypi.python.org/pypi/jdatetime/
Other
336 stars 47 forks source link

__add__ and __sub__ should return NotImplemented #84

Closed saber-solooki closed 1 year ago

saber-solooki commented 3 years ago

You should return NotImplemented in arithmetic operations. Suppose I am using a library and the object of a class in this library knows how to do arithmetic operations and this object is the right object of arithmetic operations. (suppose this expression: a + b. "a" is an instance of jdatetime and "b" is the object of my library). When you raise an exception, the interpreter cannot reach to radd method of the object of the class in my library("b" in my example).

Related links: NotImplemented Implementing the arithmetic operations

farzadghanei commented 3 years ago
$ python
Python 3.9.5 (default, May 14 2021, 00:00:00) 
[GCC 10.3.1 20210422 (Red Hat 10.3.1-1)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from datetime import datetime
>>> now = datetime.now()
>>> now
datetime.datetime(2021, 7, 2, 8, 59, 41, 891375)
>>> now + 1
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unsupported operand type(s) for +: 'datetime.datetime' and 'int'
>>> 

python's datetime raises an error. this project aims to be as compatible as possible:

jdatetime is Jalali implementation of Python's datetime module

with some extra functionality for local/calendar reasons only. I don't think this library needs to implement the proposed behavior here, as that behavior is designed for numeric types. Not every type is a number.

saber-solooki commented 3 years ago

You get raised exception because numeric types also don't know how to perform + operand. If you pay more attention to my example, "b" which is an object of a specific class (not the object of numeric types) that knows how to perform arithmetic operations of a datetime object, cannot do its duty because you already raise an exception. Besides if you read my mentioned links carefully (I mentioned bellow):

If A falls back to the boilerplate code, and it were to return a value from add(), we’d miss the possibility that B defines a more intelligent radd(), so the boilerplate should return NotImplemented from add(). (Or A may not implement add() at all.)

the python document emphasizes returning NotImpelmented when you don't know how to perform the arithmetic operations, not raising TypeError.

farzadghanei commented 3 years ago

@saber-solooki could you please share some code using python datetime and your library, that works?

I read the linked articles carefully, and it's clear it's discussing about numeric types, not every type. but there are other types like timedelta that can do arithmetic operations with date times.

farzadghanei commented 3 years ago

@saber-solooki you're right, the standard library datetime module does return NotImplemented

>>> import datetime
>>> datetime.datetime.now().__add__('foo')
NotImplemented

Maybe you're interested in making a PR for this?

saber-solooki commented 3 years ago

I give you this example. Look at this line of celery library. start parameter is an instance of datetime and ends_in is an instance of ffwd class. As you see ffwd class implement __radd__ function. datetime doesn't know how to add ffwd object to datetime and return NotImpelemted but ffwd object knows how to add datetime object to itself which implemented in __radd__. Because you raise an exception in __add__ function of jdatetime, ffwd object doesn't get a chance to try its implementation.

saber-solooki commented 3 years ago

I will be glad to make a PR for this. Thank's

farzadghanei commented 3 years ago

Yes, current behavior of jdatetime (raising) differs that of stdlib datetime (returning), so a PR to make it right is more than welcome, thanks.

As a side note, about the celery example you sent, if an application is using jdatetime with celery and it reaches there and fails because of this difference, implementing it won't solve the issue, because ffwd only accepts datetime.date objects, which jdatetime is not.

so I think the solution for that particular problem, is to convert the jdatetime instance to a datetime when working with celery.

saber-solooki commented 3 years ago

Yes, you are right. I don't know what is your reason to not inherit from datetime. I handled mentioned problem with a trick