アルゴリズム、OTP桁数、タイムステップ数が変更できることの確認対応 by NMai-source · Pull Request #10 · ykws/OneTimePasswordExample

NMai-source · 2021-10-26T14:27:27Z

対応する Issue が Linked Issues に設定されていることを確認してください

対応内容

ライブラリ OneTimePassword の仕様検証
アルゴリズム、OTP桁数、タイムステップ数が変更できることの確認

スクリーンショット

before	after

Closes #2

ykws · 2021-10-26T14:38:03Z

@NMai-source まだ作業中かもしれませんが、今後その場合は Draft として作成できるのでそのようにしてください。

レビューする前に、コンフリクトしているのでこれを解消する必要があります。
まずコミットの数が PR の内容に合っていないので、以下のいずれかの対応を試してみてください。

この PR のマージ先を ykws:main ではなく ykws:feature/settings にする
Feature/settings #7 の PR を main にマージする

これにより、 #7 の差分はこの PR の差分としては除外される想定になります。

NMai-source · 2021-10-26T14:50:27Z

@ykws
２の対応について、この場合のmainは自分のリポジトリのmainのことですか？
それともykws:mainにマージできるような操作方法がありますか？

ykws · 2021-10-26T15:09:38Z

@NMai-source 失念していました。私のリポジトリなので、 #7 の PR に Merge ボタン表示されてなかったですよね？

今 Collaborator としてこのリポジトリに招待したので後ほど承認をお願いします。
先行して #7 はこちらでマージしました。

しかし、依然コンフリクトが残っているのとコミット数が多いので rebase をする必要がありそうです。

remote branch に upstream を追加してもらっていれば、 git fetch upstream && git rebase upstream/main できれいになる想定です。
#1 (comment)

ykws · 2021-10-27T13:35:23Z

スクリーンショットの Markdown はこのように書くと視認性が高いです

| before | after |
|-------|------|
| ![image](https://user-images.githubusercontent.com/84489601/138899650-d98e4bd1-188f-436b-bc69-0819bb2a9015.png) | ![image](https://user-images.githubusercontent.com/84489601/138899342-597213bb-43c6-4ae2-8f71-c424a438563a.png) |

before	after

また Simulator のカメラアイコン（赤矢印）でスクリーンショットを撮影することができ、こちらだとスクリーンのみのキャプチャができます。
ショートカットキーは ⌘S です。 ⌘R で録画も開始できます。

Macのスクショ機能	Simulatorのスクショ機能

NMai-source · 2021-10-28T14:15:14Z

リポジトリへの招待とマージありがとうございます。
git fetch upstream && git rebase upstream/mainを実行し、コンフリクトなくマージはできたのですが、
肝心の#2対応の修正部位のコミットがなくなってしまったので、確認し直します。
（git histでログを見ていた際はコミットが残っているように見えたので、見逃してしまいました）

NMai-source · 2021-10-28T14:30:43Z

PRコメントについて、書き方を教えて頂きありがとうございます。
修正しました。

ykws · 2021-10-28T14:50:23Z

肝心の#2対応の修正部位のコミットがなくなってしまったので、確認し直します。

確かに消えてますね。
NMai-source 側の main を rebase して feature-#2 として git push -f してしまえば良い気がします。
@NMai-source

NMai-source · 2021-10-29T13:39:53Z

@ykws

NMai-source 側の main を rebase して feature-#2 として git push -f してしまえば良い気がします。

こちらの対応で修正分を盛り込んだ形でrebaseできました。

ykws

rename
アルゴリズム変更の反映
タイムステップ数変更の描画反映

PR のタイトルは適切な内容で設定してほしいです。
設定画面に戻ると毎回設定がリセットされてしまい使いづらいので、TOP画面と設定画面では同じ値を参照するようにして欲しいです。
設定値はローカルに保存してしまっても良いと思います。

NMai-source · 2021-11-09T06:05:07Z

@ykws
お疲れ様です。
以下の指摘について、対応が完了しました。

rename
アルゴリズム変更の反映
タイムステップ数変更の描画反映
PR のタイトルは適切な内容で設定してほしいです。

以下の指摘についてはこれから修正予定です。

設定画面に戻ると毎回設定がリセットされてしまい使いづらいので、TOP画面と設定画面では同じ値を参照するようにして欲しいです。
設定値はローカルに保存してしまっても良いと思います。

ykws

指摘事項対応されていることを確認できました！

コミットメッセージに以下のような記載は避けるようにしましょう。

PR指摘事項対応

後からログを見た時に PR を知らないでも変更の意図がわかるようになっているのが望ましいです。

ykws · 2021-11-09T11:57:10Z

-    if (_test1 != nil) {
-        self.token.algorithm = _test1;
+    self.scheduledTimerWithTimeInterval = 0.03f;
+    if (_algorithm != nil) {


元がそうなのですが、なるべく self を使うと良いです。

ykws · 2021-11-09T12:00:37Z


 #import "ViewController.h"
 #import "OneTimePassword.h"
+#import "SettingController.h"


ViewController から SettingController は見てないので、 import しなくて良いはずです。

ykws · 2021-11-09T12:07:04Z

+    self.token = [OTPToken tokenWithType:OTPTokenTypeTimer secret:secretData name:name issuer:issuer];
+    self.scheduledTimerWithTimeInterval = 0.03f;
+    if (_algorithm != nil) {
+        NSArray *OTPAlgorithmStrings = @[@"SHA1", @"SHA256", @"SHA512"];


この辺りは、 SettingController と冗長なので Model を利用して表現できると良いです。

ykws · 2021-11-09T12:08:35Z


    _localProgress = newProgress;
    [self.oneTimePasswordProgressView setProgress:_localProgress animated:NO];
+


余計な空行はなるべく増やさないでおきましょう。

ykws · 2021-11-09T12:08:56Z


 @implementation ViewController

+


余計な空行はなるべく増やさないでおきましょう。

ykws · 2021-11-09T12:10:25Z

-
 @property (nonatomic, assign) CGFloat localProgress;
-
+@property (nonatomic) float scheduledTimerWithTimeInterval;


逆に意図して入れてある空行は残しておくと良いです。

ykws · 2021-11-09T12:10:58Z


 @property (weak, nonatomic) IBOutlet UILabel *oneTimePasswordLabel;
 @property (weak, nonatomic) IBOutlet UAProgressView *oneTimePasswordProgressView;
-


逆に意図して入れてある空行は残しておくと良いです。

ykws · 2021-11-09T12:13:02Z

 #import <UIKit/UIKit.h>

 @interface ViewController : UIViewController
-


逆に意図して入れてある空行は残しておくと良いです。

ykws · 2021-11-09T12:16:14Z

-    self.token = [OTPToken tokenWithType:OTPTokenTypeTimer secret:secretData name:name issuer:issuer ];
+    self.token = [OTPToken tokenWithType:OTPTokenTypeTimer secret:secretData name:name issuer:issuer];
+    self.scheduledTimerWithTimeInterval = 0.03f;
+    if (_algorithm != nil) {


また nil のケースは考慮したくないので、デフォルト値で初期化できておくと良いと思います。

ykws · 2021-11-09T13:00:05Z

+        if([_period isEqualToString:@"60"]) {
+            self.scheduledTimerWithTimeInterval = 0.06f;
+        }


文字列よりも intValue から算出可能なので、以下のように求めつつ、プロパティにせずに直接 NSTimer に渡しても良さそうです。

Suggested change

if([_period isEqualToString:@"60"]) {

self.scheduledTimerWithTimeInterval = 0.06f;

}

self.scheduledTimerWithTimeInterval = self.token.period / 1000;

ykws · 2021-11-11T12:06:29Z

@NMai-source f9456f5 マージコミットが含まれてしまっているので、可能ならこのタイミングで rebase をするとログがより見やすくなります。

練習用のリポジトリを用意したので、この辺りの感覚を掴んでもらえると嬉しいです。

https://github.com/ykws/git-rebase-example

NMai-source · 2021-11-11T13:49:02Z

@ykws
マージコミットについて承知いたしました。
練習用のリポジトリについて、練習してみようと思ってできていないので、
issueを進めるのと並行してやろうと思います。

ykws assigned NMai-source Oct 26, 2021

ykws added the enhancement New feature or request label Oct 26, 2021

ykws mentioned this pull request Oct 27, 2021

iOS TOTP OSS 仕様準拠の確認 #1

Closed

NMai-source force-pushed the feature-#2 branch from 11eac51 to 5e543b8 Compare October 28, 2021 13:59

NMai-source and others added 7 commits October 28, 2021 23:58

設定画面を追加

57eb73c

アルゴリズムを選択できるようにする　ドラムの行数の指定でエラー

977e215

アルゴリズムを選択できるようにする画面作成完了

ef44b2c

アルゴリズムを選択できるようにする対応

e00de01

アルゴリズムを択できるようにする対応

105e7bb

アルゴリズムを選択できるようにする対応　値の受け渡し完了

381b8ee

プロパティ未設定時の対応

f62e1e6

NMai-source force-pushed the feature-#2 branch from 5e543b8 to f62e1e6 Compare October 29, 2021 13:34

ykws requested changes Oct 30, 2021

View reviewed changes

ykws reviewed Nov 3, 2021

View reviewed changes

Comment thread OneTimePasswordExample/SettingController.m

Comment thread OneTimePasswordExample/SettingController.h Outdated

ykws reviewed Nov 5, 2021

View reviewed changes

Comment thread OneTimePasswordExample/SettingController.m

Comment thread OneTimePasswordExample/SettingController.h Outdated

PR指摘事項修正

8605a17

NMai-source changed the title ~~Feature #2~~ アルゴリズム、OTP桁数、タイムステップ数が変更できることの確認対応 Nov 9, 2021

ykws approved these changes Nov 9, 2021

View reviewed changes

Merge branch 'main' into feature-ykws#2

f9456f5

NMai-source merged commit 57545f8 into ykws:main Nov 10, 2021

This was referenced Nov 11, 2021

設定を保存する #15

Closed

property のアクセスに self を利用する #16

Closed

Model を追加する #17

Closed

フォーマット修正 #18

Closed

初期値を設定する #19

Closed


		_localProgress = newProgress;
		[self.oneTimePasswordProgressView setProgress:_localProgress animated:NO];


		@property (nonatomic, assign) CGFloat localProgress;

		@property (nonatomic) float scheduledTimerWithTimeInterval;


		@property (weak, nonatomic) IBOutlet UILabel *oneTimePasswordLabel;
		@property (weak, nonatomic) IBOutlet UAProgressView *oneTimePasswordProgressView;

		#import <UIKit/UIKit.h>

		@interface ViewController : UIViewController

Conversation

NMai-source commented Oct 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

対応内容

スクリーンショット

Uh oh!

ykws commented Oct 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NMai-source commented Oct 26, 2021

Uh oh!

ykws commented Oct 26, 2021

Uh oh!

ykws commented Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NMai-source commented Oct 28, 2021

Uh oh!

NMai-source commented Oct 28, 2021

Uh oh!

ykws commented Oct 28, 2021

Uh oh!

NMai-source commented Oct 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ykws left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NMai-source commented Nov 9, 2021

Uh oh!

ykws left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ykws commented Nov 11, 2021

Uh oh!

NMai-source commented Nov 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

NMai-source commented Oct 26, 2021 •

edited

Loading

ykws commented Oct 26, 2021 •

edited

Loading

ykws commented Oct 27, 2021 •

edited

Loading

NMai-source commented Oct 29, 2021 •

edited

Loading