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

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

Open
sshishov opened this issue May 5, 2024 · 10 comments
Assignees

Comments

@sshishov
Copy link
Contributor

sshishov commented May 5, 2024

  • Faker version: 25.0.1
  • OS: MacOS 14.4.1 (23E224)

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]
@sshishov sshishov changed the title pydecimal - min_value and max_value should support Decimal, along with float pydecimal() - min_value and max_value should support Decimal, along with float May 5, 2024
@stefan6419846
Copy link
Contributor

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
Copy link
Contributor Author

sshishov commented May 5, 2024

@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
Copy link
Contributor

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

@parsariyahi
Copy link
Contributor

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
Copy link
Contributor Author

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

@parsariyahi
Copy link
Contributor

parsariyahi commented May 28, 2024

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
Copy link
Collaborator

fcurella commented Jun 3, 2024

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

@parsariyahi
Copy link
Contributor

parsariyahi commented Jun 3, 2024

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

@fcurella
Copy link
Collaborator

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
Copy link

jamescooke commented Sep 17, 2024

@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 🙏🏻

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

No branches or pull requests

5 participants