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

Fix code at head, pytype errors, and small Event refactoring #50

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

jeremywhelchel
Copy link

Code was broken at head after yesterday's Event dataclass refactoring.

pytype would have caught this error. So I fixed all the outstanding pytype errors in anticipation of (hopefully) adding some automated pytype checking to the repo.

@callebtc
Copy link
Contributor

callebtc commented Feb 7, 2023

Ended up fixing this as well here: callebtc#5.

The from_dict() method is needed or the Event() argument error will probably happen again some time.

This is a great PR. LGTM.

@jeremywhelchel
Copy link
Author

Thanks @callebtc

@jeffthibault WDYT?

could be three separate prs:

Looking for feedback before doing anything else here

FWIW, here is the relevant pytype failures wrt the Event() breakage:

File "/private/tmp/python-nostr/nostr/relay.py", line 117, in _is_valid_message: Function Event.__init__ expects 1 arg(s), got 8 [wrong-arg-count]
         Expected: (self, content, public_key, created_at, kind, tags, signature)
  Actually passed: (self, content, public_key, created_at, kind, tags, signature, _)

File "/private/tmp/python-nostr/nostr/message_pool.py", line 58, in _process_message: Function Event.__init__ expects 1 arg(s), got 8 [wrong-arg-count]
         Expected: (self, content, public_key, created_at, kind, tags, signature)
  Actually passed: (self, content, public_key, created_at, kind, tags, signature, _)

I'm a big fan of using pytype to catch these breakages.

@blankey1337
Copy link

Bumping this one, would love to see it get merged.

@earonesty
Copy link

been 2 months since a merge. guessing @jeffthibault is asleep. thinking either (a) fork or (b) ask jeff to promote a couple maintainers... clearly no lack of people want to help

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

Successfully merging this pull request may close these issues.

4 participants