marshmallow-code / apispec-webframeworks

Web framework plugins for apispec (formally in apispec.ext).
MIT License
32 stars 23 forks source link

Add type hints #103

Closed kasium closed 11 months ago

kasium commented 2 years ago

Similar to apispec, can you please consider to add typehints? For that a new release of apispec releasing the type hints would be great!

lafrech commented 2 years ago

Sure, I can release apispec.

Would you like to contribute type hints here?

lafrech commented 2 years ago

I just released apispec 5.2.0.

I won't work on type hints in this repo but PR welcome!

Thanks.

kasium commented 2 years ago

@lafrech thanks for the release. I'll try to open a PR next week

kasium commented 2 years ago

@lafrech I face the following issue. In e.g. flask.py path_helper is defined as

def path_helper(self, operations: dict, *, view, app=None, **kwargs: Any) -> str:

The BasePlugin defines it as

def operation_helper(
        self,
        path: str | None = None,
        operations: dict | None = None,
        **kwargs: typing.Any,
    ) -> None:

Can you please support?

lafrech commented 2 years ago

You pasted operation_helper but anyway you're right, there's a mismatch.

https://github.com/marshmallow-code/apispec/blob/eda2887adb8baf414b280ce5bd5528e9614eaa66/src/apispec/plugin.py#L53-L59

    def path_helper(
        self,
        path: str | None = None,
        operations: dict | None = None,
        parameters: list[dict] | None = None,
        **kwargs: typing.Any,
    ) -> None:

It is called here:

        for plugin in self.plugins:
            try:
                ret = plugin.path_helper(
                    path=path, operations=operations, parameters=parameters, **kwargs
                )

path is definitely the first positional argument. The definition here is wrong but it works because although path and operations are positional, they are called by name.

The right thing to do would be to fix this here by adding a path argument we won't use (or should it be _ or _path?).

Besides, the annotations are wrong in apispec since the return value should be hinted as str | None I guess. Perhaps a fix to add to https://github.com/marshmallow-code/apispec/pull/765. I'm not familiar with typing and even less with mypy and I'm a git surprised this isn't caught in the tests. I'm wondering what the tests actually test.

kasium commented 2 years ago

Hi, it would be great if you could fix that in your open change https://github.com/marshmallow-code/apispec/pull/765.

The reason why mypy doesn't complain is quite simple: plugin is here Any, since self.plugins is set as list or tuple w/o specifing the type as list[Plugin] | tuple[plugin]. This could be put to your change as well.

The addition to the path_helper implementations can be done by me

lafrech commented 2 years ago

@kasium I updated https://github.com/marshmallow-code/apispec/pull/765.

kasium commented 2 years ago

@lafrech another question: In flask.py you use operations.update(yaml_utils.load_operations_from_docstring(view.__doc__)). However, the docstring of a callable can be None. For now I can just write assert view.__doc__ to silence the error or should I add an exception?

lafrech commented 2 years ago

Right. This was overlooked. I don't mind either way. An exception with an explicit message is nice. An assert with an explicit comment in the code does the job.