XRPLF / xrpl-py

A Python library to interact with the XRP Ledger (XRPL) blockchain
ISC License
150 stars 84 forks source link

Use kw_only=True parameter in dataclass module #673

Closed ckeshava closed 3 months ago

ckeshava commented 8 months ago

High Level Overview of Change

Introduces the use of kw_only=True parameter in the dataclass module. It preserves backwards compatibility for older versions of Python, because this feature is enabled in Python v3.10 or later versions

Context of Change

The use of @require_kwargs_on_init decorator prevents IDE's from providing auto-complete suggestions. The newer versions of Python standard library allow developers to solve this issue using kw_only=True in the constructor of dataclass. Reference documentation: https://docs.python.org/3.10/library/dataclasses.html#module-contents

Type of Change

Did you update CHANGELOG.md?

This change is based on the suggestion in this Stackoverflow page: https://stackoverflow.com/questions/72733998/kw-only-and-slots-dataclass-compatibility-with-older-versions-of-python

Thanks for the tip @mvadari !

mvadari commented 8 months ago

Does this preserve the error message in 3.7?

ckeshava commented 8 months ago

Yes, developers will get an error like:

----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/ckeshavabs/xrpl-py/tests/unit/models/requests/test_channel_authorize.py", line 13, in test_has_secret_only_is_valid
    request = ChannelAuthorize(
  File "/Users/ckeshavabs/xrpl-py/xrpl/models/utils.py", line 59, in new_init
    raise XRPLModelException(
xrpl.models.exceptions.XRPLModelException: ChannelAuthorize.__init__ only allows keyword arguments. Found the following positional arguments: (20,)

----------------------------------------------------------------------
Ran 6 tests in 0.001s

FAILED (errors=1)
➜  xrpl-py git:(kwOnly) ✗ python3 --version
Python 3.7.17
➜  xrpl-py git:(kwOnly) ✗ 
mvadari commented 8 months ago

Please add a unit test to confirm that an error is thrown in previous versions.

ckeshava commented 8 months ago

@mvadari 👍 I've added unit tests to Sign and ChannelAuthorize classes: tests/unit/models/requests/test_channel_authorize.py and tests/unit/models/requests/test_sign.py

fixed in commit: b0d0ffaa6bab8b259c5bad8973661ef1bcfb52de

ckeshava commented 7 months ago

@mvadari I have added the unit tests, would you be able to check this PR again?

JST5000 commented 7 months ago

I would add this to the HISTORY.md since the intent is to make auto-complete work with IDEs (which will impact developers)

ckeshava commented 6 months ago

@mvadari could you take a look at this PR? do you suggest any other changes?

ckeshava commented 6 months ago

@khancode @justinr1234 could you guys take a look at this PR at your convinience?

ckeshava commented 4 months ago

Please let me know if there are any objections. I'd like to merge this PR