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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 11 additions & 15 deletions multipart/multipart.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,19 +18,20 @@
if TYPE_CHECKING: # pragma: no cover
from typing import Callable, Protocol, TypedDict

class QuerystringCallbacks(TypedDict, total=False):
class BaseCallbacks(TypedDict, total=False):
on_end: Callable[[], None]

class QuerystringCallbacks(BaseCallbacks, total=False):
on_field_start: Callable[[], None]
on_field_name: Callable[[bytes, int, int], None]
on_field_data: Callable[[bytes, int, int], None]
on_field_end: Callable[[], None]
on_end: Callable[[], None]

class OctetStreamCallbacks(TypedDict, total=False):
class OctetStreamCallbacks(BaseCallbacks, total=False):
on_start: Callable[[], None]
on_data: Callable[[bytes, int, int], None]
on_end: Callable[[], None]

class MultipartCallbacks(TypedDict, total=False):
class MultipartCallbacks(BaseCallbacks, total=False):
on_part_begin: Callable[[], None]
on_part_data: Callable[[bytes, int, int], None]
on_part_end: Callable[[], None]
Expand All @@ -39,7 +40,6 @@ class MultipartCallbacks(TypedDict, total=False):
on_header_value: Callable[[bytes, int, int], None]
on_header_end: Callable[[], None]
on_headers_finished: Callable[[], None]
on_end: Callable[[], None]

class FormParserConfig(TypedDict):
UPLOAD_DIR: str | None
Expand Down Expand Up @@ -608,8 +608,9 @@ class BaseParser:
performance.
"""

def __init__(self) -> None:
def __init__(self, callbacks: BaseCallbacks) -> None:
self.logger = logging.getLogger(__name__)
self.callbacks = callbacks

def callback(self, name: str, data: bytes | None = None, start: int | None = None, end: int | None = None):
"""This function calls a provided callback with some data. If the
Expand Down Expand Up @@ -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?

self._started = False

if not isinstance(max_size, Number) or max_size < 1:
Expand Down Expand Up @@ -795,12 +795,10 @@ class QuerystringParser(BaseParser):
def __init__(
self, callbacks: QuerystringCallbacks = {}, strict_parsing: bool = False, max_size: float = float("inf")
) -> None:
super().__init__()
super().__init__(callbacks)
self.state = QuerystringState.BEFORE_FIELD
self._found_sep = False

self.callbacks = callbacks

# Max-size stuff
if not isinstance(max_size, Number) or max_size < 1:
raise ValueError("max_size must be a positive number, not %r" % max_size)
Expand Down Expand Up @@ -1055,12 +1053,10 @@ def __init__(
self, boundary: bytes | str, callbacks: MultipartCallbacks = {}, max_size: float = float("inf")
) -> None:
# Initialize parser state.
super().__init__()
super().__init__(callbacks)
self.state = MultipartState.START
self.index = self.flags = 0

self.callbacks = callbacks

if not isinstance(max_size, Number) or max_size < 1:
raise ValueError("max_size must be a positive number, not %r" % max_size)
self.max_size = max_size
Expand Down
4 changes: 2 additions & 2 deletions tests/test_multipart.py
Original file line number Diff line number Diff line change
Expand Up @@ -290,8 +290,8 @@ def test_handles_rfc_2231(self):

class TestBaseParser(unittest.TestCase):
def setUp(self):
self.b = BaseParser()
self.b.callbacks = {}
callbacks = {}
self.b = BaseParser(callbacks)

def test_callbacks(self):
called = 0
Expand Down