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

整理: TypeAdapter によるバリデーション・ダンプを共通化 #1440

Closed
wants to merge 3 commits into from

Conversation

tarepan
Copy link
Contributor

@tarepan tarepan commented Jun 26, 2024

内容

概要: TypeAdapter によるバリデーション・ダンプを共通化してリファクタリング

現在の ENGINE では「外部由来の bytes/object ⇔ 内部向け構造体」の変換に TypeAdapter を利用している。各構造体向けにそれぞれ TypeAdapter インスタンスが作られ、メソッドを用いて変換(validation/dump)がおこなわれている。これにより安全な変換が可能になっている。
この変換時に TypeAdapter インスタンスを直接利用しているため、各モジュールは pydantic に依存している。本質的にここではバリデーションとダンプがしたいのであり、pydantic は実装詳細に過ぎない。また pydantic 以外のライブラリであってもいい。
幸いこれらの変換は内部向け構造体に依らず共通の処理をしている。ゆえに validate_x() / dump_x() のような関数を各構造体向けに生成しその内部で pydantic を利用すれば pydantic の影響範囲を小さく出来る。pydantic を差し替える際も生成関数 1 箇所に変更を加えるだけで済む。

このような背景から、TypeAdapter によるバリデーション・ダンプを共通化するリファクタリングを提案します。

関連 Issue

無し

@tarepan tarepan requested a review from a team as a code owner June 26, 2024 14:53
@tarepan tarepan requested review from Hiroshiba and removed request for a team June 26, 2024 14:53
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

ちょっとややこしくなっているなと感じました。
それプラス、若干早すぎる一般化に思えるかもで、そのままでも良いのかもと感じました。

なにか先の見通しがあっての変更であればお聞きしたみ。

追記:あるいは名前のややこしさが解消したらそれでも

Comment on lines -61 to +70
_save_format_dict_adapter = TypeAdapter(dict[str, SaveFormatUserDictWord])
_validate_obj_as_save_format_dict = generate_obj_validator(
dict[str, SaveFormatUserDictWord]
)
_dump_save_format_dict_as_bytes = generate_bytes_dumper(
dict[str, SaveFormatUserDictWord]
)
Copy link
Member

Choose a reason for hiding this comment

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

1回で良い処理を2回やってるかもです。
あとちょっと名前もややこしいかも。

@tarepan
Copy link
Contributor Author

tarepan commented Jun 27, 2024

名前のややこしさ

Before/After が以下のようになっています:

# Before
_env_adapter = TypeAdapter(Envs)
args = _cli_args_adapter.validate_python(args_dict)

# After
_validate_obj_as_envs = generate_obj_validator(Envs)
args = _validate_obj_as_cli_args(args_dict)

_python が「Pythonの…何?」となりそうであったためオブジェクト(obj)へ変更し、他は既存の名称をなぞるように命名しています。
どの辺がややこしく感じるでしょうか?(あるいは元からややこしい名前という話?)

@Hiroshiba
Copy link
Member

Hiroshiba commented Jun 27, 2024

元もややこしいとは思います。

_dump_save_format_dict_as_bytes辺りが動詞2回続いてややこしくなってそうだなと。

TypeAdapterをラップするクラスを作り、dump_jsondump_bytesvalidate_objなどをメソッドとして実装するとかすればややこしさは軽減できると思います。
同じTypeAdapterを2回作ってる部分も削減できるかもです。(処理速度的に気にしてませんが)
目的と合ってるか微妙かもですが・・・。

わりと元のままでも良いなら、元のままにしておきつつ別のとこに時間割くのもありかもです。

@tarepan
Copy link
Contributor Author

tarepan commented Jun 28, 2024

_dump_save_format_dict_as_bytes辺りが動詞2回続いてややこしくなってそう

👍️
妥当な指摘です。

TypeAdapterをラップするクラスを作り

クラスの方が見通しが良い、という視点が新鮮でした。たしかにメソッド名は単純になりそうです。

元のままにしておきつつ別のとこに時間割くのもありかも

👍️
将来 pydantic の利用範囲を極小化したくなった場合に re-open できればと思います。
本 PR は NoGo につき close します。

@tarepan tarepan closed this Jun 28, 2024
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.

None yet

2 participants