Replace attrdict with NamedTuple #76

cburgdorf · 2019-12-20T12:05:22Z

What was wrong?

The usage of attrdict dependency was causing a deprecation warning.

  from collections import (
/home/cburgdorf/Documents/hacking/ef/trinity/venv/lib/python3.7/site-packages/attrdict/mixins.py:5: DeprecationWarning: Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated, and in 3.8 it will stop working

How was it fixed?

Replaced with NamedTuple which should have the positive side effect of a stronger guarantee of immutability.

Cute Animal Picture

cburgdorf · 2019-12-20T12:13:17Z

review ping @pipermerriam

pipermerriam

This would be a breaking change and while I'm 👍 on moving this direction, I don't think we an do it immediately. It doesn't appear that this is fixed upstream, nor has there been a release in about 11 months.

I'd recommend pursuing an upstream fix for this first and seeing if the library maintainers can get it merged and released so we can address this without a major bump.

Second option is to use something like this and vendor it for the time being: https://github.com/ethereum/web3.py/blob/c1c80dba4d9c203af7a4963bf1ce5e922f1172da/web3/datastructures.py

I'm good with us eventually switching to a NamedTuple but I consider this library to be somewhat foundational and thus a major version bump in it is going to need to be taken carefully and probably shouldn't happen until sometime in January when all this holiday nonsense has passed.

cburgdorf · 2019-12-23T10:19:06Z

It doesn't appear that this is fixed upstream, nor has there been a release in about 11 months.

The inactivity there was part of the reason why I believe it is not worth to try to get this fixed upstream. I wouldn't expect to this to result in an upstream release in a short time frame which is why I thought it is better to fix it on our on end. Especially if we consider that modern Python has no need for that library any more.

Second option is to use something like this and vendor it for the time being: https://github.com/ethereum/web3.py/blob/c1c80dba4d9c203af7a4963bf1ce5e922f1172da/web3/datastructures.py

But wouldn't that be as much of a breaking change as just going with NamedTuple? I'm assuming the breaking part is just the underlying object that is being returned and maybe the repr_pretty which I could still put back in. Other than that, the solution proposed in the PR is attribute access compatible.

Someone calling signTransaction can still access signed_tx.rawTransaction or any other attribute like before. I might be missing some other subtle breaking change?

I'm good with us eventually switching to a NamedTuple but I consider this library to be somewhat foundational and thus a major version bump in it is going to need to be taken carefully and probably shouldn't happen until sometime in January when all this holiday nonsense has passed.

Yeah, no rush. I'd probably say, let's just wait and cut a new major release some time in January.

pipermerriam · 2019-12-23T16:40:20Z

It is a breaking change because named tuples don't support dictionary style key retrieval. Whatever we return needs to support both attribute and string-based key lookups via the standard Mapping interface to retain backwards compat.

cburgdorf · 2019-12-23T16:48:01Z

string-based key lookups

Ah! That makes sense. Thanks for clarifying.

veox · 2020-01-07T19:08:22Z

2c: probably means there needs to be a test for this.

As to the previous

The inactivity there was part of the reason why I believe it is not worth to try to get this fixed upstream. I wouldn't expect to this to result in an upstream release in a short time frame which is why I thought it is better to fix it on our on end.

There will be no release: the ustream repo has been archived by the owner.

On a brighter side, the deprecation has been moved to Python 3.9 since original post:

/home/veox/src/trinity/.virtualenv/trinity/lib/python3.7/site-packages/eth_account/account.py:1:
    DeprecationWarning: Using or importing the ABCs from 'collections' instead of from 'collections.abc'
    is deprecated since Python 3.3,and in 3.9 it will stop working

cburgdorf · 2020-02-06T15:58:27Z

There will be no release: the ustream repo has been archived by the owner.

Considering that the project seems dead, I'd argue we should just cut a new major release and migrate Trinity to it. We can continue to backport further changes into the 0.x development line if the community demands it, but considering that this breaking change is really tiny and easy to resolve I do not actually see that even becoming an issue.

cburgdorf · 2020-04-17T11:11:22Z

I have updated the change to preserve the current behavior of providing index access. I also tuned the tests to cover indexing access. What do you think about this @pipermerriam ?

I'd love to get this fixed as it's currently the main driver of nasty warnings in Trinity.

pipermerriam · 2020-04-17T14:16:41Z

@carver any thoughts on the breaking change here. I'm inclined to since we're still in the 0.x release line and thus I believe we're allowed to introduce breaking changes in minor point releases.

cburgdorf · 2020-04-17T14:25:22Z

@pipermerriam what's the remaining breaking change now that I added index access support?

carver · 2020-04-17T18:16:14Z

eth_account/account.py

@@ -507,7 +508,7 @@ def sign_message(self, signable_message: SignableMessage, private_key):
        :param private_key: the key to sign the message with
        :type private_key: hex str, bytes, int or :class:`eth_keys.datatypes.PrivateKey`
        :returns: Various details about the signature - most importantly the fields: v, r, and s
-        :rtype: ~eth_account.datastructures.AttributeDict
+        :rtype: ~eth_account.datastructures.SignedMessage


Interesting that the doctests (which were supposedly checked recently) didn't catch that the returned value changes below.

I would expect there to be a number of places in the docs where this would change.

I just spent 1,5 hour to depuzzle this and here's the best I know so far:

We never had any doctest running in this repo so far. I wasn't able to find a single CI run that ran any doctests (even though they exist)

Then I found Doctests for Account and other general doc cleanup #94 which seems to fix this (it reports running 50 tests)

I'd happily rebase this on top of #94 but in case #94 needs more time to finish it should be easy to rebase that one on top of these changes, too.

🤦 I totally lost track of #94. I just updated and it should be ready to merge. @cburgdorf I'm happy to pull your changes into #94, or vice versa, whatever is easier! Just let me know.

@kclowes I don't have merge rights in this repo. If you have merge rights then feel free to merge both PRs in any preferred order.

carver · 2020-04-17T18:19:41Z

@pipermerriam what's the remaining breaking change now that I added index access support?

For very type-conscious consumers, it could still break their tests. Maybe their mypy tests or doctests. (Maybe even projects like web3.py?)

I have always preferred a more duck-typing approach, but with the strict-typing tactic used in a lot of our projects, we might still be forced to do it is a v0 minor release (which we have been using to indicate a breaking change).

I still think it's a good change to keep the indexing access to minimize the cost of updating downstream projects. 👍

pipermerriam

👍 didn't see that the breaking change had been fixed. 👍

pipermerriam · 2020-04-21T14:11:44Z

@cburgdorf I merged the other PR, mind rebasing to make sure everything is still good here and then I'll get this merged.

cburgdorf · 2020-04-21T14:59:27Z

@pipermerriam rebased and ready to 🚢

pipermerriam · 2020-04-21T15:05:41Z

@carver can you handle the release?

carver · 2020-04-21T19:25:14Z

Seems like a fuzzy gray area of incompatibility. I'll just be sure to highlight it in the release notes. I just checked web3.py, and it doesn't have any reference to AttributeDict being returned by eth-account AFAICT.

So I'll go ahead and release this as a patch release.

carver · 2020-04-22T22:35:58Z

It's a little late today, so I'll aim for Thursday morning.

* remove gitter, testing setup, and pandoc sections, add quotes to dev install

cburgdorf mentioned this pull request Dec 20, 2019

Fix deprecation warnings on startup ethereum/trinity#1414

Closed

6 tasks

cburgdorf force-pushed the christoph/refactor/named_tuple branch from 24654d5 to 007e8ea Compare December 20, 2019 12:11

pipermerriam requested changes Dec 20, 2019

View reviewed changes

cburgdorf force-pushed the christoph/refactor/named_tuple branch 2 times, most recently from 322feb6 to 5f2e7b2 Compare April 17, 2020 11:09

carver reviewed Apr 17, 2020

View reviewed changes

pipermerriam approved these changes Apr 17, 2020

View reviewed changes

cburgdorf force-pushed the christoph/refactor/named_tuple branch from 5f2e7b2 to f1d68de Compare April 20, 2020 08:22

cburgdorf added 2 commits April 21, 2020 16:44

Ignore whole .idea directory

b4a88bb

Replace attrdict with NamedTuple

94dabda

cburgdorf force-pushed the christoph/refactor/named_tuple branch from f1d68de to 94dabda Compare April 21, 2020 14:52

pipermerriam merged commit 8cb6582 into ethereum:master Apr 21, 2020

pipermerriam mentioned this pull request May 19, 2020

Typing of AttributeDict return values is broken ethereum/web3.py#1656

Closed

pacrob added a commit to pacrob/eth-account that referenced this pull request Apr 26, 2023

remove outdated sections of README (ethereum#76)

96a6759

* remove gitter, testing setup, and pandoc sections, add quotes to dev install

pacrob added a commit to pacrob/eth-account that referenced this pull request Jan 30, 2024

remove outdated sections of README (ethereum#76)

91d93ef

* remove gitter, testing setup, and pandoc sections, add quotes to dev install

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace attrdict with NamedTuple #76

Replace attrdict with NamedTuple #76

cburgdorf commented Dec 20, 2019

cburgdorf commented Dec 20, 2019

pipermerriam left a comment

cburgdorf commented Dec 23, 2019

pipermerriam commented Dec 23, 2019

cburgdorf commented Dec 23, 2019

veox commented Jan 7, 2020 •

edited

Loading

cburgdorf commented Feb 6, 2020

cburgdorf commented Apr 17, 2020

pipermerriam commented Apr 17, 2020

cburgdorf commented Apr 17, 2020

carver Apr 17, 2020

cburgdorf Apr 20, 2020

kclowes Apr 20, 2020

cburgdorf Apr 21, 2020

carver commented Apr 17, 2020 •

edited

Loading

pipermerriam left a comment

pipermerriam commented Apr 21, 2020

cburgdorf commented Apr 21, 2020

pipermerriam commented Apr 21, 2020

carver commented Apr 21, 2020 •

edited

Loading

carver commented Apr 22, 2020

Replace attrdict with NamedTuple #76

Replace attrdict with NamedTuple #76

Conversation

cburgdorf commented Dec 20, 2019

What was wrong?

How was it fixed?

Cute Animal Picture

cburgdorf commented Dec 20, 2019

pipermerriam left a comment

Choose a reason for hiding this comment

cburgdorf commented Dec 23, 2019

pipermerriam commented Dec 23, 2019

cburgdorf commented Dec 23, 2019

veox commented Jan 7, 2020 • edited Loading

cburgdorf commented Feb 6, 2020

cburgdorf commented Apr 17, 2020

pipermerriam commented Apr 17, 2020

cburgdorf commented Apr 17, 2020

carver Apr 17, 2020

Choose a reason for hiding this comment

cburgdorf Apr 20, 2020

Choose a reason for hiding this comment

kclowes Apr 20, 2020

Choose a reason for hiding this comment

cburgdorf Apr 21, 2020

Choose a reason for hiding this comment

carver commented Apr 17, 2020 • edited Loading

pipermerriam left a comment

Choose a reason for hiding this comment

pipermerriam commented Apr 21, 2020

cburgdorf commented Apr 21, 2020

pipermerriam commented Apr 21, 2020

carver commented Apr 21, 2020 • edited Loading

carver commented Apr 22, 2020

veox commented Jan 7, 2020 •

edited

Loading

carver commented Apr 17, 2020 •

edited

Loading

carver commented Apr 21, 2020 •

edited

Loading