Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#306: support Selenium #508

Merged
merged 4 commits into from
Aug 31, 2019
Merged

#306: support Selenium #508

merged 4 commits into from
Aug 31, 2019

Conversation

kmyk
Copy link
Member

@kmyk kmyk commented Aug 30, 2019

close #306
close #382
login に Selenium を使えるようにします。つまり、ログインを普通の Chrome や Firefox のような web ブラウザ経由でできるようにします。

  • SNS 認証とかが使えるようになるのでうれしい。メンテの手間も減る。
  • ターミナル上にパスワードを入力するのはこわいけど、ブラウザ上の正規のフォームなら抵抗感は減るはずなのでその点でも有利。
  • ブラウザとの連携という都合上、動かすのはちょっと面倒。なのですべて置き換えられるわけではない。どうせ WebDriver のインストールの作業があるので、 seleniumsetup.py の依存に追加していない
  • ドキュメントは書いておいた方がいいけれど、自動で起動する設定なので必須ではないはず。とりあえず後まわしにします
  • yukicoder への login ができなかったので書きました。これが終わったら developmaster にマージさせます。もう延期はないはず

@kmyk kmyk requested a review from fukatani August 30, 2019 20:38
@fukatani
Copy link
Contributor

こんなことができるんですね!便利です。

already logged inでも、チェックされずにログイン認証が始まる仕様でしたっけ?
というのと、loginに成功したのかよくわからないですね。
(前からでしたっけ?)

最後にis_logged_in確認してできてたらメッセージを出したほうが安心できますがどうでしょう?

Copy link
Contributor

@fukatani fukatani left a comment

Choose a reason for hiding this comment

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

基本的にOKですが、質問させていただきました。

@kmyk
Copy link
Member Author

kmyk commented Aug 31, 2019

指摘ありがとうございます。ログインを試す前後にログイン済み判定をしてメッセージを出すようにしました。こういうのは使い勝手にけっこう影響するので助かります。

@fukatani
Copy link
Contributor

旧版は「本当にこれでできているのかな?」という気持ちにはなりました。レビュアー目線だからかもしれませんが、、

@fukatani fukatani merged commit e54e309 into develop Aug 31, 2019
@fukatani fukatani deleted the issue/306 branch August 31, 2019 13:34
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.

2 participants