Skip to content

Conversation

@LazyGon
Copy link

@LazyGon LazyGon commented Nov 8, 2020

フラットファイル実装を維持しつつMySQLとSQLiteに対応しました。

すでにうっちーさんが作っていたスキーマではちょっと不足のようでしたのでこちらでデザインし直しました。
UndineMailerDBScheme.pdf

基本的な実装方針としては、Yamlに依存している部分を抽象化してデータベース保存実装を拡張するというものです。

データ移行はまだほんの少ししか実装してませんが、そこ以外はデバッグが終わったのでPRしています。

ucchyocean and others added 30 commits October 23, 2016 22:09
and slight revision of `EditmodeTipsMessage`
isSent result is reversed, which leads unexpected error
- remove EditmodeMails when mail is sent
- Removed meaningless check
that plugin do not show created group in flatfile mode
SQL sytax error
when using sqlite. (MySQL ignores it by default)
SQL invalid column reference
to behave like v1
- Cannot specify "where" inside "on duplicate key"
- field name typo
@LazyGon LazyGon marked this pull request as draft November 8, 2020 09:35
@LazyGon LazyGon marked this pull request as ready for review November 8, 2020 17:01
@LazyGon
Copy link
Author

LazyGon commented Nov 10, 2020

結構大きめのPRなので

  • 追加内容が多すぎてレビューが大変
  • レビューが大変なうえに追加コードにバグがありそうで信用できない
  • これまでのデータ構造ではなくまったく新しい構造を導入したいので、以前の構造を維持しているこの実装では困る
  • ベースのコードは自らの手で実装したい

というような場合は、このデータベースコードを参考にして本流側で頑張ってもらい、このPRはクローズしてもらうのでも構いません。
ご随意に扱ってください。

(なにやら、なぜかこのPRに関係ない所の話で私について何か言っているコメントが書かれたという通知メールが来ましたが、これについて弁明しておきます。
かなり事実無根の話です。私がしたPRで特定不可能な大量のバグが含まれていたという証拠も、受け入れが無茶なほど巨大なPRをそのまま受け入れろとせがんだこともありません。
(PRが大きいと言われれば、可能な限り分割してPRを再作成までして対応しています。このPRではほぼ一から実装してるのでナンセンスすぎてそこまでできませんが)

ほかの方にもメールが来たと思うので一応こちらで言わせていただきました。この部分はこのPRに関係ないので、無視して構いません。
私に要件があるならこのPRではなく私のツイッターでお願いします。プロフに書いといたので。)

勘違いでした。失礼しました!!(恥を勢いでごまかす顔)

@RoboMWM
Copy link
Contributor

RoboMWM commented Nov 10, 2020

Yes I commented but after a minute later I realized that this PR was for a development branch of a major version release, rendering most of my comment void. So I just removed my comment. Sorry about the confusion.

(And btw I think I added that you are active which is a good thing. I was talking about a couple PRs from another guy on another project (see 43 and 44 on AcmeProject/WildernessTp))

@ucchyocean
Copy link
Owner

@LazyGon さん、
PRありがとうございます。
修正量の多さに、僕も面食らっていますが、いつものように全行動作確認とコードレビューしてから導入可否の検討をしてみますね。少々お時間ください。
あと、彼はあなたのPRに対して言っているのではなく、「彼が今まで運営してきたOpenSourceCodeプロジェクトの知見として、大きなPRを受け入れたときに不具合が多数あって苦労したことがある」という経験から、知見者としてアドバイスをくれているだけですね。

@RoboMWM
Thank you for your helpful advice. LazyGon has made PR to my repository several times, so I trust and rely on him.
I will follow your advice, test this PR well, and then consider whether it can be implemented.

@LazyGon
Copy link
Author

LazyGon commented Nov 11, 2020

@ucchyocean
なるべくコード量を減らすように、かつ元のコードを変更しないように努力したのですが、さすがにデータベース実装を付け加えて動くようにするとなると私の腕ではこれぐらいのコード量が必要でした...。
(もしかするとdatabaseパッケージ内のテーブルアクセスクラスを抽象化したりしてもうちょっと減らせそうではあります)

動作確認は、なるべくこちらでもゲーム内でほとんどの機能をデバッグして動くことは確認しましたが、それでも見つけられなかったコードのバグがあるかもしれませんで、先に謝っておきますm(_ _)m

Javadocが不足だったり、どういうコード書いてるのかわからないなどがありましたら是非お伝えください。

@RoboMWM

I was talking about a couple PRs from another guy on another project

I re-checked mail and certainly that's not me. Where are my eyes? 🙄
Usually before reading English, I use Google translate to grab overview (because I'm not native).
That was NG :p

Thanks for advice :)

@ucchyocean ucchyocean self-requested a review November 14, 2020 16:41
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.

4 participants