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(typing): Typing fixes #18

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

fix(typing): Typing fixes #18

wants to merge 5 commits into from

Conversation

Potato1682
Copy link
Member

レビューを承認する前に入念に確認してください。見落としがあるかもしれません。

  • anyの代わりにAnyを使用
    - any() を指定していることになっています。
  • Import __future__.annotations for compatibility
    - 互換性維持のため。
  • Use | operator instead of Union[]
    - __future__.annotations が適用されている状態でこの記法ができます。
  • Only use Any if Any is used in the union
    - Any はすべての型を通すため、 UnionOptional である意味がありません。 Any を使用したくない場合は代わりの型をレビューでリクエストしてください。
  • Fix super().__init__() arguments
    - 一部 super().__init__() の引数が間違っていた箇所があります。
  • None check if required
    - None がチェックされていない箇所にNoneチェックを入れました。
  • Use ABC base class to AbstractHandlerBase, use @abstractmethod to the class's methods
    - ABC が使用されていないため入れましたが、なにか理由があれば修正します。

以下の推奨事項があります。修正願います。

  • TypedDict を使用した dict のキーの固定
  • tuple[] の型の設定
  • Any が使用されている箇所の代替となる型の指定
  • Authorizationヘッダーのチェック部分でのNoneチェックの実装

- Use `Any` instead of `any`
- Import `__future__.annotations` for compatibility
- Use `|` operator instead of `Union[]`
- Only use `Any` if `Any` is used in the union
- Fix `super().__init__()` arguments
- None check if required
- Use `ABC` base class to `AbstractHandlerBase`, use `@abstractmethod` to the class's methods
@Potato1682 Potato1682 added documentation Improvements or additions to documentation enhancement New feature or request labels Sep 26, 2021
@Potato1682 Potato1682 marked this pull request as draft September 26, 2021 13:17
@Potato1682 Potato1682 marked this pull request as ready for review September 26, 2021 13:28
src/server/handler.py Show resolved Hide resolved
@@ -501,7 +510,7 @@ def make_cache(self) -> None:
cursor[method] = EndPoint(method, rt, path, function, auth, args, bool(paths), docs)
self.count += 1

def get_endpoint(self, method: str, path: str, params: Optional[dict] = None) -> Optional[EndPoint]:
def get_endpoint(self, method: str, path: str, params: dict = {}) -> Optional[EndPoint]:
Copy link
Member

Choose a reason for hiding this comment

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

引数のデフォルト値が mutable です。

path = parse.urlparse(self.request.path)
queries = dict(parse.parse_qsl(path.query))

if self.request.method in ["GET", "HEAD", "TRACE", "OPTIONS"]:

self.call_handler(path.path, {}, queries)
else:
if "Content-Type" in self.request.headers:
if self.request.headers is not None and "Content-Type" in self.request.headers:
Copy link
Member

Choose a reason for hiding this comment

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

self.request.headers がNoneになることはありません。

@@ -81,34 +86,43 @@
header_limit = 100


class AbstractHandlerBase(StreamRequestHandler):
class AbstractHandlerBase(ABC, StreamRequestHandler):
Copy link
Member

Choose a reason for hiding this comment

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

Abstractである必要がありません

Copy link
Member Author

Choose a reason for hiding this comment

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

このクラスからの継承を止めて、子クラスで維持させた方がいいかもしれません。
全部の子クラスで使用されている関数があればabcを使用します。

src/endpoint.py Outdated
@@ -1,5 +1,7 @@
from __future__ import annotations
from typing import Union, Optional

from typing import Optional, Any, Literal
Copy link
Member

Choose a reason for hiding this comment

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

Python3.7での互換性を保てません。

@@ -277,7 +292,7 @@ def _header(self, data: str) -> None:
if len(kv) != 2:
raise ParseException("MALFORMED_HEADER")

self._response.headers.add(*kv)
self._response.headers.add(*kv) if self._response.headers is not None else None
Copy link
Member

Choose a reason for hiding this comment

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

self._response.headersが None になることはありません。

Copy link
Member Author

Choose a reason for hiding this comment

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

Noneチェックが必要ない型にするには__init__からNone定義をしている箇所を削除する必要があります。

self.multiple = True
elif req.headers["Connection"] == "close":
self.multiple = False
if req.headers is not None:
Copy link
Member

Choose a reason for hiding this comment

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

req.headersがNoneになることはありません。


def handle_switch(self):
try:
if self.request is None:
Copy link
Member

Choose a reason for hiding this comment

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

self.request がNoneになることはありません。

@@ -149,18 +160,24 @@ def log_request(self, **kwargs):
if not no_req_log:
self.logger.info(get_log_name(), '%s -- %s %s -- "%s %s"' %
(kwargs["client"], kwargs["code"], "" if kwargs["message"] is None else kwargs["message"],
self.request.method, kwargs["path"]))
self.request.method if self.request is not None else "<no method>", kwargs["path"]))
Copy link
Member

Choose a reason for hiding this comment

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

self.request がNoneになることはありません。

ep = endpoint.loader.get_endpoint(self.request.method, path, path_param)
ep: Optional[endpoint.EndPoint] = None

if self.request is not None and self.request.method is not None:
Copy link
Member

Choose a reason for hiding this comment

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

self.requestself.request.methodNoneになることはありません。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants