feat: #868 Django formsの導入#893
Conversation
- subekashi/forms.pyを新規作成 - ContactForm: category(ChoiceField) + detail(CharField) - SongDeleteForm: reason(CharField) - SongEditForm: title/authors/url/imitate/lyrics + boolean 6フィールド - contact.py, song_delete.py, song_edit.pyでformを使用してバリデーションを置き換え - song_edit.html: name="is-*" → name="is_*" に変更(SongEditFormのフィールド名に合わせる) - song_new.pyはフィールド名に-auto/-manualサフィックスがある複合構造のためスコープ外 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
PR Review: Django formsの導入 (#868)全体的に良い方向性のリファクタリングです。手動バリデーションをDjango formsに移行する意図は明確で、コードの可読性が向上しています。いくつか気になる点を挙げます。 バグ・懸念点1. song_delete.py: form.errors['reason'][0] の直接キーアクセス SongDeleteForm には現在 reason フィールドしかないので問題は出ませんが、将来フィールドが追加されたとき(またはnon-field errorが発生したとき)に KeyError になるリスクがあります。より堅牢な書き方: 2. song_edit.py: next(iter(form.errors.values()))[0] というパターン Python 3.7以降はdict挿入順が保証されるため動作しますが、意図が伝わりにくいコードです。SongEditForm ではフォーム定義順に title → authors と並んでいますが、フォームの定義順を変えると表示されるエラーが変わります。 3. contact.py: バリデーションエラーの詳細が失われる 元のコードも同様のメッセージでしたが、ChoiceField の invalid_choice エラーなどフォームレベルで設定した詳細なエラーメッセージが表示されません。song_delete.py と同様に form.errors からメッセージを取り出すか、現状の汎用メッセージで意図的に統一するかを決めると良いと思います。 細かい点4. song_edit.py: バリデーション順序の変更 元のコードはURLバリデーション → 作者 → タイトルの順でしたが、今回の変更でDjango formsのバリデーションが先に走るため、タイトル/作者のエラーがURL不正より優先して表示されるようになります。動作仕様としては問題ありませんが、意図した変更かどうか確認してください。 5. authors フィールドの空白のみ入力 元のコードにあった if not authors_input.strip(): のチェックは、Django の CharField がデフォルトで strip=True なため等価に動作します。意図通りの実装です。 テストカバレッジsubekashi/forms.py に対するユニットテストが追加されていません。既存の subekashi/tests/ ディレクトリにテストを追加することを推奨します。特に:
良かった点
総じて良いPRですが、懸念点1・2の堅牢性と、テストの追加を検討いただけると完成度が上がると思います。 |
概要
subekashi/forms.pyを新規作成し、バリデーションをDjango formsに移行。変更内容
forms.py(新規)
ContactFormcategory(ChoiceField),detail(CharField)contact.pySongDeleteFormreason(CharField)song_delete.pySongEditFormtitle,authors,url,imitate,lyrics, boolean×6song_edit.py各フィールドに日本語エラーメッセージを設定。
ビューの変更
request.POST.get()による手動バリデーション →form.is_valid()+form.cleaned_dataに置き換えテンプレートの変更
song_edit.html:name="is-original"等のハイフン名 →name="is_original"等のアンダースコア名に変更(SongEditFormのフィールド名との整合)スコープ外
song_new.py: フィールド名がis-original-auto/is-original-manualと複合構造になっており、2フォーム1ページ構成なため対象外Test plan
🤖 Generated with Claude Code