-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add user_message
to Web3Exception
#3263
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
Conversation
758385c
to
5e29809
Compare
050af92
to
149d9f0
Compare
I like it! Could probably use a test or 2 though, no? |
@pacrob Yeah I can add tests that throw this and assert on the |
149d9f0
to
e46051f
Compare
e46051f
to
91ec094
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice change, lgtm 👍🏼
web3/exceptions.py
Outdated
): | ||
super().__init__(*args) | ||
# Assign properties of Web3Exception | ||
self.__dict__.update(locals()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed how we were updating this the first time around. Is there a reason we are using locals()
here instead of assigning self.user_message = user_message
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not use user_message explicitly, though I could. locals()
here also saves the other args and kwargs on the Web3Exception
. kwargs
are not supported by Exception
as far as I could tell so this felt like a good place to keep that data.
Also thought keeping it open to anything the user would want to store might be useful. If we want to enforce it strictly to user_message
I am fine doing so. Not sure if there are risks to saving any named property on the Web3Exception
.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll always lean toward easier reading of the code / debugging as long as there's not a big performance hit. Since explicit property assignment is slightly more performant I'd just lean that way. To me this feels like I don't know what the class is inheriting when I look at it and that would take up more dev time to fix a bug, etc.
That said, if you feel strongly about it I can be convinced :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree with having that more explicit. I guess any class properties could be overwritten, so protecting against that might be best.
I do not feel strongly about supporting kwargs
in general. It might be convenient to store some data in the exception but maybe we wait until a use case comes about. Do you think there's significant value to storing custom kwargs
somehow? What about args
, since they are on the Exception
it may or may not be useful to have them accessible on both.
I'm on board with setting the user_message
explicitly. Let me know what you think about kwargs
and args
for Web3Exception
.
91ec094
to
7a60c3e
Compare
7a60c3e
to
c9fea5e
Compare
c9fea5e
to
eeaa3cc
Compare
eeaa3cc
to
3183013
Compare
3183013
to
ddb2810
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
What was wrong?
Closes #2798
Related to #3094
In #2796 there was a recommendation to add
user_message
to theWeb3Exception
class to support hint messages and other information for users.How was it fixed?
Implemented an optional
user_message
named argument inWeb3Exception
. This adds the ability to provide a more human friendly message.Todo:
Cute Animal Picture