Skip to content
This repository has been archived by the owner on Apr 12, 2023. It is now read-only.

ログ出力に関する変更 #148 #225 #226

Merged
merged 12 commits into from
Sep 8, 2021

Conversation

Takym
Copy link
Contributor

@Takym Takym commented Jun 11, 2021

Issue 番号

目的

  • 例外の握り潰しを無くしログ出力する様にしました。
  • DebugLogger/FFImageLoadingLogger のログ出力先を LoggerService に変更しました。

破壊的変更をもたらしますか

[ ] Yes
[x] No

Pull Request の種類

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Documentation content changes
[x] Other... Please describe: Logging

コードの入手

git clone https://github.com/Takym/cocoa.git
cd cocoa
git checkout logging_148_225
dotnet restore

確認事項

  • アプリが正常に動作する。
  • 漏れが無いか確認する。

その他

  • 全てのコードを確認したわけではありませんので、漏れがある可能性があります。

@Takym Takym marked this pull request as ready for review June 11, 2021 06:43
@Takym Takym marked this pull request as draft June 11, 2021 06:47
@Takym Takym marked this pull request as ready for review June 11, 2021 06:51
@Takym Takym changed the title ログ出力に関する変更 ログ出力に関する変更 #148 #225 Jun 11, 2021
@keiji keiji self-requested a review June 11, 2021 08:32
using System;
using System.Collections.Generic;
using Covid19Radar.Services.Logs;
using FFImageLoading.Helpers;
using Prism.Logging;

namespace Covid19Radar.Services
{
public class FFImageLoadingLogger : IMiniLogger
Copy link
Collaborator

Choose a reason for hiding this comment

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

このクラスって今使われていますか?
もし使われていなければ手を付けずに不要なクラスとして削除した方がいいかもしれません。

Copy link
Contributor Author

@Takym Takym Jun 13, 2021

Choose a reason for hiding this comment

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

このクラスは使われていないという指摘はありましたね。DebugLogger と同じインターフェースを持っており違いもよく分かりません。廃統合して Logs 名前空間に移転するのが良いのかもしれません。

Copy link
Collaborator

Choose a reason for hiding this comment

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

このPull Requestでは掲題の「ログ出力」に関するものに限定して、統廃合や移転や削除については別の機会にしましょう。
ひとまず、変更を戻して、ここはレビューをしなくても良いようにするのが良いと思います。

Copy link
Contributor Author

@Takym Takym Jun 14, 2021

Choose a reason for hiding this comment

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

打消しを行うコミットを(手動で)作成しました。1816508
(DebugLogger の変更を巻き戻さないため)

Copy link
Collaborator

@keiji keiji 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 1 to 9
/* This Source Code Form is subject to the terms of the Mozilla Public
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at https://mozilla.org/MPL/2.0/. */

using FFImageLoading.Helpers;
using Prism.Logging;
using System;
using System.Collections.Generic;
using Covid19Radar.Services.Logs;
using FFImageLoading.Helpers;
using Prism.Logging;
Copy link
Collaborator

Choose a reason for hiding this comment

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

このファイルについてはdiffが出ないようにしておいて欲しいので、このあたりも打ち消しお願いします!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

すみません。手動で書き換えたので見落としていました。

Comment on lines 14 to 15
private ILogger _logger { get; }
private readonly ILogger _logger;
Copy link
Collaborator

Choose a reason for hiding this comment

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

ここも、消したら一緒なので戻しておいてください

@Takym
Copy link
Contributor Author

Takym commented Jun 14, 2021

差分を無くすためにBOMを追加しましたが、不要でしたら取り消します。

@keiji keiji self-requested a review June 14, 2021 04:46
@b-wind
Copy link

b-wind commented Jun 14, 2021

FFImageLoadingLogger.cs 自体は #115 で削除対象に含まれているようですね。

@Takym
Copy link
Contributor Author

Takym commented Jun 14, 2021

#115 に既に含まれているのであれば #225 で話していた Issue は建てる必要は無さそうですね。

@keiji さん
#225 (comment) で話されていた Android 側での DebugLogger の設定は行った方が良いでしょうか?

Copy link
Contributor Author

@Takym Takym left a comment

Choose a reason for hiding this comment

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

DebugLogger を改善しました。ご確認お願い致します。

Comment on lines 21 to 27
private ILoggerService? GetLogger()
{
if (_logger is null) {
_logger = DependencyService.Resolve<ILoggerService>();
}
return _logger;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

最初はコンストラクタから ILoggerService を受け取っていましたが、ビルドエラーが発生したため DependencyService.Resolve<T>() を使ってサービスを取得する様に変更しました。しかし、初期化前のタイミングでは DependencyService.Resolve<T>()null を返していましたので、最初のログ出力時に取得する様に変更しました。

@Takym
Copy link
Contributor Author

Takym commented Jun 23, 2021

#225 でも書きましたが、DebugLogger/FFImageLoadingLogger を削除し、ログ出力先を Debug.WriteLine から LoggerService.Debug へ修正しました。

@keiji keiji changed the base branch from develop to feature/log_improvement June 23, 2021 08:27
@keiji
Copy link
Collaborator

keiji commented Jun 23, 2021

ブランチの向き先変更しました。
CIを動かすために一度Closeとreopenします。

@keiji keiji closed this Jun 23, 2021
@keiji keiji reopened this Jun 23, 2021
@Takym
Copy link
Contributor Author

Takym commented Jun 23, 2021

ありがとうございます。

@Takym
Copy link
Contributor Author

Takym commented Jun 23, 2021

CI が一部失敗している様ですので後程確認します。

@Takym
Copy link
Contributor Author

Takym commented Jun 23, 2021

修正しました。DebugLogger への参照の削除を忘れていたのが原因でした。

Copy link
Collaborator

@keiji keiji left a comment

Choose a reason for hiding this comment

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

LGTM

@keiji keiji requested a review from cocoa-dev July 13, 2021 00:33
@keiji keiji added waiting-for-confirmation 関係者に確認中のもの and removed waiting-for-confirmation 関係者に確認中のもの labels Jul 13, 2021
@keiji keiji merged commit aa83a25 into cocoa-mhlw:feature/log_improvement Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants