Use kw_only=True parameter in dataclass module #673

ckeshava · 2024-01-13T00:38:02Z

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

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Tests (You added tests for code that already exists, or your new feature included in this PR)
Documentation Updates
Release

Did you update CHANGELOG.md?

Yes
No, this change does not impact library users

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 !

…rds compatibility for older versions of Python, this feature is enabled in Python v3.10 or later versions

mvadari · 2024-01-16T15:12:08Z

Does this preserve the error message in 3.7?

ckeshava · 2024-01-16T18:50:45Z

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 · 2024-01-19T16:22:42Z

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

…by positional args on data class

ckeshava · 2024-01-19T16:56:24Z

@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: b0d0ffa

ckeshava · 2024-02-08T22:38:46Z

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

tests/unit/models/requests/test_channel_authorize.py

xrpl/models/utils.py

JST5000 · 2024-02-12T23:15:05Z

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

Co-authored-by: Jackson Mills <jmills@ripple.com>

mvadari · 2024-02-13T20:17:50Z

tests/unit/models/requests/test_channel_authorize.py

@@ -56,3 +56,14 @@ def test_has_multiple_signing_methods_is_invalid(self):
                passphrase=_DUMMY_STRING,
                seed_hex=_DUMMY_STRING,
            )
+
+    def test_throws_if_positional_args_mixed_with_non_positional_args(self):


Why is this test here?

I wanted a unit test that shows that positional arguments are not permitted in the constructor of ledger objects. that was my intention

Updated in 0cbc07e
let me know if that looks ok

mvadari · 2024-02-13T20:18:00Z

tests/unit/models/requests/test_sign.py

@@ -72,3 +72,13 @@ def test_valid_seed(self):
            transaction=_TRANSACTION, seed=_SEED, key_type=CryptoAlgorithm.SECP256K1
        )
        self.assertTrue(request.is_valid())
+
+    def test_ctor_has_positional_args(self):


Why is this test here?

Also, what does ctor mean?

i shortened constructor to ctor. i can use the full name

updated in 0cbc07e.
Let me know if that looks ok

tools/generate_tx_models.py

Co-authored-by: Mayukha Vadari <mvadari@ripple.com>

CHANGELOG.md

mvadari · 2024-02-14T17:11:14Z

tests/unit/models/test_utils.py

@@ -11,3 +32,24 @@ class TestUtils(TestCase):
    def test_kwargs_req(self):
        with self.assertRaises(XRPLModelException):
            IssuedCurrency(currency, issuer)
+
+    def test_throws_if_positional_args_mixed_with_non_positional_args(self):
+        with self.assertRaises(XRPLModelException):


I'm confused why this works with >=3.10 - shouldn't it be throwing a different error?

yes, thanks for pointing this out. I had forgotten to remove the decorator @required_kw_args. It was falling back on the old code. I'll update this

The old decorator should still be used in some form, to keep support for the error for <3.10.

ckeshava · 2024-02-14T20:17:19Z

tests/unit/models/test_utils.py


 class TestUtils(TestCase):
    def test_kwargs_req(self):
-        with self.assertRaises(XRPLModelException):
-            IssuedCurrency(currency, issuer)
+        if sys.version_info.major == 3 and sys.version_info.minor < 10:


@mvadari I have updated the unit tests to depend on the version of Python

I recommend writing a helper function to do this.

Also, for consistency (and in case they backport the changes), I'd recommend doing the same check you do in KW_ONLY_DATACLASS.

ckeshava · 2024-02-14T20:18:25Z

xrpl/models/utils.py

-    # noinspection PyTypeHints
-    cls.__init__ = new_init  # type: ignore
-
+    # For Python v3.10 and above, the KW_ONLY attribute in data_class


@mvadari I came up with this conditional statement to accomodate different versions of python. Does this look good? Do you have any other ideas in mind?

For consistency (and in case they backport the changes), I'd recommend doing the same check you do in KW_ONLY_DATACLASS

mvadari · 2024-02-14T21:13:52Z

tests/unit/models/test_utils.py

+    def test_positional_args_in_model_constructor_throws(self):
+        if sys.version_info.major == 3 and sys.version_info.minor < 10:
+            with self.assertRaises(XRPLModelException):
+                Sign(


Please use a different RPC for this, since Sign may be deprecated in the library at some point (it's really only used with an admin connection).

mvadari · 2024-02-15T15:45:06Z

tests/unit/models/test_utils.py

+        if is_kw_only_attr_defined_in_dataclass():
+            with self.assertRaises(XRPLModelException):
+                IssuedCurrency(currency, issuer)
+        else:
+            with self.assertRaises(TypeError):
+                IssuedCurrency(currency, issuer)


I recommend putting this if-else block in a reusable context manager to avoid needing it in every test/avoid repetition.

What do you mean by a re-usable context manager? Do you mean something like:

def testWrapper(pythonVersionDiscriminator, oldVersion, newVersion): if pythonVersionDiscriminator(): <some steps in the unit tests> <raise XRPLModelException> else: <some more steps> <raise TypeError>

?

https://book.pythontips.com/en/latest/context_managers.html

thanks for the resource. let me know if my usage looks nice.

mvadari · 2024-02-15T15:45:30Z

xrpl/models/base_model.py

@@ -73,7 +74,8 @@ def _value_to_json(value: XRPL_VALUE_TYPE) -> XRPL_VALUE_TYPE:
    return value


-@dataclass(frozen=True)
+# @require_kwargs_on_init
+@dataclass(frozen=True, **KW_ONLY_DATACLASS)


Does BaseModel need this?

I've removed it 👍

xrpl/models/utils.py

mvadari · 2024-02-15T15:46:24Z

xrpl/models/utils.py

 # Code source for requiring kwargs:
 # https://gist.github.com/mikeholler/4be180627d3f8fceb55704b729464adb

 _T = TypeVar("_T")
 _Self = TypeVar("_Self")


+def is_kw_only_attr_defined_in_dataclass() -> bool:


This should be a private method. It's fine to still use it in tests.

mvadari · 2024-02-15T15:46:37Z

xrpl/models/utils.py

+
+# Unit tests that validate this behavior can be found at test_channel_authorize.py
+# and test_sign.py files.
+KW_ONLY_DATACLASS = dict(kw_only=True) if "kw_only" in dataclass.__kwdefaults__ else {}


This should use the helper function is_kw_only_attr_defined_in_dataclass.

mvadari · 2024-02-15T15:47:05Z

xrpl/models/utils.py

+    # When support for older versions of Python (earlier than v3.10) is removed, the
+    # usage of require_kwargs_on_init decorator on model classes can also be removed.
+    if is_kw_only_attr_defined_in_dataclass():
+        # noinspection PyTypeHints


What is this comment for? I don't think PyTypeHints is something this library uses.

Okay. I was trying to preserve the existing code. #noinspection PyTypeHints is also used in the present code base. I've removed it in the latest tip of this branch 👍

Co-authored-by: Mayukha Vadari <mvadari@ripple.com>

…unction definition

… readable

JST5000

lgtm!

ckeshava · 2024-03-13T17:07:56Z

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

ckeshava · 2024-03-25T19:18:27Z

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

ckeshava · 2024-05-08T17:31:59Z

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

khancode

LGTM

use the kw_only=True parameter in the dataclass module. Ensure backwards compatibility for older versions of Python, this feature is enabled in Python v3.10 or later versions --------- Co-authored-by: Jackson Mills <jmills@ripple.com> Co-authored-by: Mayukha Vadari <mvadari@ripple.com>

use the kw_only=True parameter in the dataclass module. Ensure backwa…

eb4b680

…rds compatibility for older versions of Python, this feature is enabled in Python v3.10 or later versions

address PR comments: add unit tests to validate exceptions triggered …

b0d0ffa

…by positional args on data class

ckeshava added 2 commits February 8, 2024 14:39

Merge branch 'main' into kwOnly

252f074

Merge branch 'main' into kwOnly

70e0325

ckeshava requested review from khancode, mvadari and justinr1234 February 12, 2024 17:50

JST5000 reviewed Feb 12, 2024

View reviewed changes

tests/unit/models/requests/test_channel_authorize.py Outdated Show resolved Hide resolved

JST5000 reviewed Feb 12, 2024

View reviewed changes

xrpl/models/utils.py Outdated Show resolved Hide resolved

ckeshava and others added 5 commits February 13, 2024 09:21

Update tests/unit/models/requests/test_channel_authorize.py

1f45e4e

Co-authored-by: Jackson Mills <jmills@ripple.com>

Include comments to explain the context of KW_ONLY change

d55dff7

Merge branch 'main' of https://github.com/XRPLF/xrpl-py into kwOnly

05b867a

include the KW_ONLY change in the CHANGELOG file

9437a3a

Merge branch 'kwOnly' of https://github.com/XRPLF/xrpl-py into kwOnly

08ca191

mvadari reviewed Feb 13, 2024

View reviewed changes

tools/generate_tx_models.py Outdated Show resolved Hide resolved

ckeshava and others added 5 commits February 13, 2024 17:34

Update tools/generate_tx_models.py

cf3ed8f

Co-authored-by: Mayukha Vadari <mvadari@ripple.com>

Merge branch 'main' into kwOnly

afc1e96

Merge branch 'main' of https://github.com/XRPLF/xrpl-py into kwOnly

a79ccb7

Merge branch 'kwOnly' of https://github.com/XRPLF/xrpl-py into kwOnly

0cbc07e

remove trailing whitespace in xrpl/models/utils.py

d7b9ab3

mvadari reviewed Feb 14, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

mvadari reviewed Feb 14, 2024

View reviewed changes

Merge branch 'kwOnly' of https://github.com/XRPLF/xrpl-py into kwOnly

2ed1196

ckeshava commented Feb 14, 2024

View reviewed changes

mvadari reviewed Feb 14, 2024

View reviewed changes

address PR comments

7cfa358

mvadari reviewed Feb 15, 2024

View reviewed changes

xrpl/models/utils.py Outdated Show resolved Hide resolved

mvadari reviewed Feb 15, 2024

View reviewed changes

ckeshava and others added 6 commits February 15, 2024 13:11

Update xrpl/models/utils.py

1563c58

Co-authored-by: Mayukha Vadari <mvadari@ripple.com>

address mayukha's PR comments

49e9527

Merge branch 'kwOnly' of https://github.com/XRPLF/xrpl-py into kwOnly

870d644

simplify and remove the double negation in the kw_only_attr utility f…

84ada4b

…unction definition

use a context manager in KW_ONLY unit tests. Makes the code much more…

587d3e7

… readable

Merge branch 'main' into kwOnly

3c5e772

JST5000 approved these changes Mar 13, 2024

View reviewed changes

Merge branch 'main' into kwOnly

7072993

ckeshava requested a review from mvadari April 8, 2024 20:08

pdp2121 approved these changes May 8, 2024

View reviewed changes

Merge branch 'main' into kwOnly

1396b73

Merge branch 'main' into kwOnly

732a20d

khancode approved these changes Jun 25, 2024

View reviewed changes

ckeshava merged commit 4779d79 into main Jun 25, 2024
21 checks passed

ckeshava deleted the kwOnly branch June 25, 2024 17:51

Use kw_only=True parameter in dataclass module #673

Use kw_only=True parameter in dataclass module #673

Conversation

ckeshava commented Jan 13, 2024

High Level Overview of Change

Context of Change

Type of Change

Did you update CHANGELOG.md?

mvadari commented Jan 16, 2024

ckeshava commented Jan 16, 2024

mvadari commented Jan 19, 2024

ckeshava commented Jan 19, 2024

ckeshava commented Feb 8, 2024

JST5000 commented Feb 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JST5000 left a comment

Choose a reason for hiding this comment

ckeshava commented Mar 13, 2024

ckeshava commented Mar 25, 2024

ckeshava commented May 8, 2024

khancode left a comment

Choose a reason for hiding this comment