search

ykws / OneTimePasswordExample

OneTimePassword Example
1 stars 1 forks source link

アルゴリズム、OTP桁数、タイムステップ数が変更できることの確認対応 #10

Closed NMai-source closed 3 years ago

NMai-source commented 3 years ago

対応内容

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

スクリーンショット

before after
simulator_screenshot_D0F25097-0852-466E-95E7-6F20C702E273 simulator_screenshot_6AF1B2B5-D58D-4B7F-B643-237CFE4A532E

Closes #2

ykws commented 3 years ago

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

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

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

  2. 7 の PR を main にマージする

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

NMai-source commented 3 years ago

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

ykws commented 3 years ago

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

Screen Shot 2021-10-27 at 0 01 36

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

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

remote branch に upstream を追加してもらっていれば、 git fetch upstream && git rebase upstream/main できれいになる想定です。 https://github.com/ykws/OneTimePasswordExample/pull/1#issuecomment-944399394

ykws commented 3 years ago

スクリーンショットの 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
image image

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

Macのスクショ機能 Simulatorのスクショ機能
Screen Shot 2021-10-27 at 22 28 38 Simulator Screen Shot - iPhone 13 - 2021-10-27 at 22 28 40
NMai-source commented 3 years ago

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

NMai-source commented 3 years ago

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

ykws commented 3 years ago

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

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

NMai-source commented 3 years ago

@ykws

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

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

NMai-source commented 3 years ago

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

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

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

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

ykws commented 3 years ago

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

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

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

NMai-source commented 3 years ago

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