joke2k / faker

Faker is a Python package that generates fake data for you.
https://faker.readthedocs.io
MIT License
17.73k stars 1.93k forks source link

pydecimal() - `min_value` and `max_value` should support `Decimal`, along with `float` #2041

Open sshishov opened 5 months ago

sshishov commented 5 months ago

After migrating to version 25+, profiding default value as Decimal for min or max values causes violation

Steps to reproduce

amount=fake.pydecimal(min_value=Decimal(1), max_value=Decimal(100000)),

Expected behavior

The should not be any error observed. The rationale - a lot of software using Decimal from the box for all calculations, in the constants etc to rely on precise double calculation. Therefore I think that it should support Decimal from the box, especially if it is returning Decimal itself.

Actual behavior

Produces this mypy error:

 error: Argument "min_value" to "pydecimal" of "Faker" has incompatible type "Decimal"; expected "float | None"  [arg-type]
stefan6419846 commented 5 months ago

This is a typing issue, no "real" bug. Feel free to provide a PR to update the corresponding type hint and regenerate the typing stubs.

sshishov commented 5 months ago

@stefan6419846 how I can report typing issues? Because we are havily using faker, and we are requiring strict typing, that's why I have noticed these errors...

If you will give me a way to report typing issues, then I will report there.

stefan6419846 commented 5 months ago

It is indeed correct to report this here, but your initial report sounded like an implementation bug and not a type hint issue.

parsariyahi commented 5 months ago

If we want to fix that, we can add more annotation in code

    def pydecimal(
        self,
        left_digits: Optional[int] = None,
        right_digits: Optional[int] = None,
        positive: bool = False,
        min_value: Optional[Union[float, int, Decimal]] = None,
        max_value: Optional[Union[float, int, Decimal]] = None,
    ) -> Decimal:

but I think a problem is, we can have another types of numbers that are subclass of int or something like this, how we can handle that in here?

sshishov commented 5 months ago

Hi @parsariyahi , any subclass should be okay, I guess. We need to check though...

parsariyahi commented 5 months ago

After some research, I found something, the built-in numbers module, this module has a class Number which is a abstract base class for all numbers I think. you can read it here: https://docs.python.org/3/library/numbers.html as the docs said:

The numbers module (PEP 3141) defines a hierarchy of numeric abstract base classes which progressively define more operations. None of the types defined in this module are intended to be instantiated.

I think it's fit the needs for this annotation

fcurella commented 5 months ago

@parsariyahi using numbers.Number seems like the right solution here. Do y ou have time to prepare a Pull Request?

parsariyahi commented 5 months ago

@fcurella Yes sure, you can asign this issue to me, I will prepare a PR

fcurella commented 4 months ago

turns out, Decimal is not part of numbers.Number.

I've tried using Union[Decimal, int, float], but looks like abs() doesn't accept Decimal, so the best I can do is to define a new type BasicNumber = [int, float]

jamescooke commented 1 month ago

@fcurella Could you share a bit more about what you wrote above?

but looks like abs() doesn't accept Decimal,

abs() says:

The argument may be an integer, a floating-point number, or an object implementing __abs__().

And we have:

In [12]: Decimal.__abs__?
Signature:      Decimal.__abs__(self, /)
Call signature: Decimal.__abs__(*args, **kwargs)
Type:           wrapper_descriptor
String form:    <slot wrapper '__abs__' of 'decimal.Decimal' objects>
Docstring:      abs(self)

Plus it's clearly possible to call abs(Decimal(-1)) and get a reasonable result.

So I'm confused why you'd say it's not supported - could you help me understand 🙏🏻