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

URI.regexp 書き直し #355

Closed
wants to merge 1 commit into from
Closed

URI.regexp 書き直し #355

wants to merge 1 commit into from

Conversation

topstone
Copy link
Contributor

Ruby 2.2 の頃より URI.regexp は obsolete のようです。一応現状でも動きますが、いずれ単なる obsolete から error になるのではないかと思います。

とりあえず downloader.rb だけ書き直し、RSpec が all green になることを確認しました。他にも以下の2か所に残っています (手を付けていません)。

  • converterbase.rb(998): data.gsub!(URI.regexp(%w(http https))) do |match|
  • illustration.rb(30): if url =~ URI.regexp

Ruby 2.2 の頃より URI.regexp は obsolete のようです。
https://docs.ruby-lang.org/ja/2.2.0/class/URI.html#S_REGEXP
@@ -89,7 +89,7 @@ def self.get_novel_section_save_dir(archive_path)
#
def self.get_target_type(target)
case target
when URI.regexp
when /^[Hh][Tt][Tt][Pp][Ss]?:\/\//
Copy link

Choose a reason for hiding this comment

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

[RuboCop] Use %r around regular expression. (see on Sider)

Rule Severity
Style/RegexpLiteral convention

References:

Copy link
Owner

Choose a reason for hiding this comment

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

@topstone
確かにこの場所に関してだけは http:// か https:// で始まってるかを確認すればいいですね

%r!¥Ahttps?://!i でいい気もしますけど
(動作確認はしてない)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

私が出す PRs は「とりあえずこれで修正されるように見えるよ」というものなので、よりよい表記があったらそちらに直して下さい。(正規表現は、自分が GAWK の頃から使っているものしか覚えていないので…。)

ここの修正については、rubocop --safe-auto-correct で自動修正されるように見えます。なお、rubocop の bug っぽいものを見つけたので、自動修正もあまりあてにしてはいけなそうです。

@topstone
Copy link
Contributor Author

すみません、rubocop 通っていませんでした。一旦 close して、また後日 PR 出します。

@topstone topstone closed this Feb 15, 2020
@topstone
Copy link
Contributor Author

現状報告です。

  1. https://narou.nyanpass.jp/ 閉鎖
  2. narou.rb 発見
  3. Ruby 2.7 で使えない (常に最新の Ruby を使いたい派なので…)
  4. PRs 提出 (自分の手元では Ruby 2.7 でも動くようになった)
  5. Ruby 2.8 以降へも移行しやすいよう、ruby -w で warning 検出
  6. まずは URI.regexp から挑戦
  7. rubocop で引っかかる
  8. rubocop --safe-auto-correct で URI.regexp も自動修正されるっぽい
  9. RSpec で error 発生 (safe 修正のはずなのに…)
  10. rubocop の bug っぽい
  11. rubocop へ bug report ←今ココ

@whiteleaf7
Copy link
Owner

rubocop --safe-auto-correct で URI.regexp も自動修正されるっぽい

ほお〜

@whiteleaf7
Copy link
Owner

Ruby 2.7では動かないと注釈しているにも関わらず、2.7 で動かないと不具合報告してくる人が現れてきてしまったので、
そろそろPR頂いたやつを反映します

@topstone
Copy link
Contributor Author

topstone commented Feb 16, 2020

Ruby 2.7では動かないと注釈しているにも関わらず、2.7 で動かないと不具合報告してくる人が現れてきてしまったので、

とりあえず、narou.gemspec の中で gem.required_ruby_version = ">=2.3.0", "<2.7.0" として narou 3.4.9 などという形で release してはいかがでしょうか。注釈に書くよりも gemspec に書く方が確実かと思います。

私の手元で勝手に (上記の形の) narou-3.4.9.gem を build して Ruby 2.7 で導入しようとしたら、ちゃんと There are no versions of narou (= 3.4.9) compatible with your Ruby & RubyGems narou requires Ruby version >= 2.3.0, < 2.7.0. The current ruby version is 2.7.0.0. と表示されました。

…と私が書いているうちに、narou 3.5.0 化が進んでいましたね。失礼しました。

@whiteleaf7
Copy link
Owner

私の手元で勝手に (上記の形の) narou-3.4.9.gem を build して Ruby 2.7 で導入しようとしたら、ちゃんと There are no versions of narou (= 3.4.9) compatible with your Ruby & RubyGems narou requires Ruby version >= 2.3.0, < 2.7.0. The current ruby version is 2.7.0.0. と表示されました。

というエラーがでてしまったのですが、という報告がくるだけに一票
結局サポートコストは変わらないのだった、完

narou 3.5.0 を gem push したのでご確認ください

@topstone
Copy link
Contributor Author

narou 3.5.0 ありがとうございます。

rubocop に出した bug report は、patch が作られて master に取り込まれましたが、rubocop 0.80.0 には間に合わなかったようです。

次回 rubocop が release されたら、よければ rubocop --safe-auto-correct を試して RSpec を回してみてください。

@topstone
Copy link
Contributor Author

topstone commented Mar 1, 2020

続き : #360

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.

None yet

2 participants