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

BaseParser should receive the callbacks dict #115

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

eltbus
Copy link
Contributor

@eltbus eltbus commented Feb 16, 2024

Opinionated PR here, but I don't think an instance should have attributes added AFTER its creation to make it work.

Also there are a couple things that I'd like to get feedback on:

  1. Using TypedDict can be misleading for users. For example:
from multipart.multipart import OctetStreamParser, OctetStreamCallbacks

def foo():
    return None

def bar(b: bytes, x: int, y: int):
    print(b, x, y)
    return None

# [OK] Explicit
explicit = OctetStreamParser(callbacks={"on_start": foo, "on_data": bar, "on_end": foo})
# =======

# [OK] From hinted variable
a: OctetStreamCallbacks = {"on_start": foo, "on_data": bar, "on_end": foo}
from_hinted_variable = OctetStreamParser(callbacks=a)
# =======

# [WARNING] From non-hinted variable
b = {"on_start": foo, "on_data": bar, "on_end": foo}
from_non_hinted_variable = OctetStreamParser(callbacks=b) # <---- WARNING HERE
# Diagnostics:
# 1. Argument of type "dict[str, Unknown]" cannot be assigned to parameter "callbacks" of type "OctetStreamCallbacks" in function "__init__"
#      "dict[str, Unknown]" is incompatible with "OctetStreamCallbacks" [reportArgumentType]

Users will get a warning even if they're doing things right because of a missing type hint!

How much should we play around TypedDict?

EDIT: no longer using Mapping or MutableMapping. Instead, use a Base Callback dict. Improves "compatibility" between BaseParser and the derived ones. Still, TypedDict complains about dict.pop when used with undefined keys.

@eltbus
Copy link
Contributor Author

eltbus commented Feb 16, 2024

One more thing: should BaseParser be an abstract class?

@@ -696,8 +697,7 @@ class OctetStreamParser(BaseParser):
"""

def __init__(self, callbacks: OctetStreamCallbacks = {}, max_size: float = float("inf")):
super().__init__()
self.callbacks = callbacks
super().__init__(callbacks)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem here is that those are not type check compliant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I am not sure I understand what you mean. I do get a warning if, for example, I replace callbacks with a 1.

In my IDE:

Diagnostics:
1. Argument of type "Literal[2]" cannot be assigned to parameter "callbacks" of type "BaseCallbacks" in function "__init__"
     "Literal[2]" is incompatible with "BaseCallbacks" [reportArgumentType]

Or running mypy:

multipart/multipart.py:700: error: Argument 1 to "__init__" of "BaseParser" has incompatible type "int"; expected "BaseCallbacks"  [arg-type]

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant on the inherited classes. If I call the super().__init__(callbacks), should error. Right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is working for me. Would definitely be better with a named arg.

Also the tests are passing, and they do initialize the different parsers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would you prefer super().__init__(callbacks=callbacks)? Or none at all?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure we need a base class here...

Copy link
Contributor Author

@eltbus eltbus Mar 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A Base class already exists, and expects both in it's description and it's methods (i.e self.callback), that an attribute self.callbacks exists.

That is why I see an error in line 595 (as of commit 3b15e38):

image

In my opinion, this BaseParser class should:

  1. Be an abstract class
  2. Avoid initialization like this:
image

It feels weird to me for an object to rely on an attribute that is not defined at all.
The method self.callback will fail if the dictionary self.callbacks is not added after initialization.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you create a PR proposing those changes?

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.

2 participants