Add new `Lint/UriRegexp` cop #4694

koic · 2017-09-02T18:22:19Z

Feature

This cop identifies places where URI.regexp can be replaced by URI::Parser.new.make_regexp.

% cat test.rb
# frozen_string_literal: true

URI.regexp
URI.regexp('http://example.com')

% bundle exec rubocop test.rb
Inspecting 1 file
W

Offenses:

/tmp/test.rb:3:5: W: Use URI::Parser.new.make_regexp instead of URI.regexp.
URI.regexp
    ^^^^^^
/tmp/test.rb:4:5: W: Use URI::Parser.new.make_regexp('http://example.com') instead of URI.regexp('http://example.com').
URI.regexp('http://example.com')
    ^^^^^^

1 file inspected, 2 offenses detected

Target Problem

Suppress the following Ruby's warnings.

% ruby -w -ruri -e 'URI.regexp'
-e:1:in `<main>': warning: URI.regexp is obsolete

Before submitting the PR make sure the following are checked:

pocke · 2017-09-03T02:32:25Z

lib/rubocop/cop/lint/uri_regexp.rb

+
+        def_node_matcher :uri_regexp_with_argument?, <<-PATTERN
+          (send
+            (const nil :URI) :regexp


It does not match ::URI.regexp.

Thanks for the review. I missed that viewpoint. I fixed by 90bc2a5.

pocke · 2017-09-03T02:38:06Z

config/enabled.yml

@@ -1403,6 +1403,10 @@ Lint/UnreachableCode:
  Description: 'Unreachable code.'
  Enabled: true

+Lint/UriRegexp:
+  Description: 'Use URI::Parser.new.make_regexp` instead of `URI.regexp`.'


I guess URI::DEFAULT_PARSER.make_regexp is better.
https://github.com/ruby/ruby/blob/4b9aeef1fa2f32a4555f88c6e83539374a1bd8d1/lib/uri/common.rb#L22

require 'benchmark' require 'uri' Benchmark.bm(20) do |x| puts 'empty arg' x.report('URI::Parser.new'){ 2000.times{URI::Parser.new.make_regexp}} x.report('URI::DEFAULT_PARSER'){2000.times{URI::DEFAULT_PARSER.make_regexp}} puts 'with arg' x.report('URI::Parser.new'){ 2000.times{URI::Parser.new.make_regexp('http://example.com')}} x.report('URI::DEFAULT_PARSER'){2000.times{URI::DEFAULT_PARSER.make_regexp('http://example.com')}} end

user system total real empty arg URI::Parser.new 2.010000 0.010000 2.020000 ( 2.016804) URI::DEFAULT_PARSER 0.000000 0.000000 0.000000 ( 0.000144) with arg URI::Parser.new 2.460000 0.000000 2.460000 ( 2.451649) URI::DEFAULT_PARSER 0.330000 0.000000 0.330000 ( 0.331841)

I see! Because it does not generate every time like URI::Parser.new, the constant URI::DEFAULT_PARSER is fast. I fixed by 88ff097.

pocke · 2017-09-03T10:07:37Z

lib/rubocop/cop/lint/uri_regexp.rb

+
+        def_node_matcher :uri_regexp_with_argument?, <<-PATTERN
+          (send
+            (const $_ :URI) :regexp


This pattern matches MyClass::URI.regexp('str') also.

$ ruby-parse -e ::URI; MyClass::URI (begin (const (cbase) :URI) (const (const nil :MyClass) :URI))

Could you change this matcher to use {nil cbase}?

Sure 💦 I fixed by ced09d3.

pocke

👍

Drenmi · 2017-09-04T08:22:41Z

lib/rubocop/cop/lint/uri_regexp.rb

+        private
+
+        def register_offense(node, top_level: '', arg: '')
+          format = format(MSG, top_level, arg, top_level, arg)


The MSG format string could use named fragments to avoid having to pass duplicate arguments. 🙂

Thank you for telling me how to write cool. I fixed by 21f5f1b.

Drenmi

🚀

koic · 2017-09-05T09:29:26Z

config/enabled.yml

@@ -1403,6 +1403,10 @@ Lint/UnreachableCode:
  Description: 'Unreachable code.'
  Enabled: true

+Lint/UriRegexp:
+  Description: 'Use URI::DEFAULT_PARSER.make_regexp` instead of `URI.regexp`.'


I'll fix this typo.

-URI::DEFAULT_PARSER.make_regexp` +`URI::DEFAULT_PARSER.make_regexp`

rrosenblum · 2017-09-05T22:04:53Z

URI.regexp can be replaced by URI::Parser.new.make_regexp

Suppress the following Ruby's warnings.

 % ruby -w -ruri -e 'URI.regexp'
-e:1:in `<main>': warning: URI.regexp is obsolete

Before I saw this, I wasn't sure why this cop was being introduce. Now, I am just a bit disappointed with Ruby. Why would they change a static method for an instance method? I like that the method has been moved to URI::Parser.

rrosenblum · 2017-09-05T22:07:39Z

spec/rubocop/cop/lint/uri_regexp_spec.rb

+
+  it 'registers an offense when using `URI.regexp` with argument' do
+    expect_offense(<<-RUBY.strip_indent)
+      URI.regexp('http://example.com')


What do you think about having the offense highlight more of the expression? It seems like this should highlight URI.regexp or URI.regexp('http://example.com')

I do not know if that is correct, but since URI module does not change, this cop focused on only regexp.

koic · 2017-09-06T03:22:58Z

URI.regexp can be replaced by URI::Parser.new.make_regexp

Thanks for your comment. I also replaced URI.regexp with URI::Parser.new.make_regexp at first. However, I learned that replacing it with URI::DEFAULT_PARSER.make_regexp is preferable from the viewpoint of performance. This PR has been changed as such now.

#4694 (comment)

bbatsov · 2017-09-10T07:38:02Z

lib/rubocop/cop/lint/uri_regexp.rb

+  module Cop
+    module Lint
+      # This cop identifies places where `URI.regexp`
+      # can be replaced by `URI::DEFAULT_PARSER.make_regexp`.


I think you should also mention that URI.regexp was deprecated and this is the reason why the cop exists.

Right now it's definitely not clear why this is being flagged.

I improved to the offense message that URI.regexp is obsolete. 00835fb
What do you think about this?

bbatsov · 2017-09-10T07:38:13Z

lib/rubocop/cop/lint/uri_regexp.rb

+      #
+      # @example
+      #   # bad
+      #   URI.regexp("http://example.com")


I'd use single quotes here.

I fixed by c5bf278.

koic force-pushed the add_new_lint_uri_regexp_cop branch 2 times, most recently from b52aa7a to cd4dc6c Compare September 2, 2017 18:30

koic changed the title ~~Add new Lint/UriRegexp cop~~ Add new Lint/UriRegexp cop Sep 2, 2017

pocke reviewed Sep 3, 2017

View reviewed changes

koic force-pushed the add_new_lint_uri_regexp_cop branch from d6df59d to 88ff097 Compare September 3, 2017 09:41

pocke reviewed Sep 3, 2017

View reviewed changes

pocke approved these changes Sep 3, 2017

View reviewed changes

koic mentioned this pull request Sep 3, 2017

Add new Performance/UriDefaultParser cop #4696

Merged

11 tasks

koic force-pushed the add_new_lint_uri_regexp_cop branch from ced09d3 to a0e6018 Compare September 4, 2017 05:39

Drenmi reviewed Sep 4, 2017

View reviewed changes

koic force-pushed the add_new_lint_uri_regexp_cop branch from b8e25e1 to 21f5f1b Compare September 4, 2017 09:31

koic mentioned this pull request Sep 4, 2017

Suppress deprecation warnings in production code rubygems/gemstash#162

Merged

Drenmi approved these changes Sep 4, 2017

View reviewed changes

koic commented Sep 5, 2017

View reviewed changes

rrosenblum reviewed Sep 5, 2017

View reviewed changes

bbatsov reviewed Sep 10, 2017

View reviewed changes

koic added 7 commits September 10, 2017 19:16

Add new Lint/UriRegexp cop

bce9d69

Match when there is a top-level double colon

c8eb0a9

Use URI::DEFAULT_PARER faster than URI::Parser.new

9355f20

Match only the expected pattern

099fa61

Remove a message arguments redundancy

c03bbc4

Fix a typo

949410f

Use single quote instead of double quote

c5bf278

koic force-pushed the add_new_lint_uri_regexp_cop branch from 06fb0ea to 2e121ea Compare September 10, 2017 11:18

Add to the offense message that URI.regexp is obsolete

00835fb

koic force-pushed the add_new_lint_uri_regexp_cop branch from 2e121ea to 00835fb Compare September 10, 2017 11:28

bbatsov merged commit c769543 into rubocop:master Sep 11, 2017

koic deleted the add_new_lint_uri_regexp_cop branch September 11, 2017 06:59

mpotter mentioned this pull request Oct 21, 2017

Upgrade to rubocop 0.51.0 ableco/ablecop#55

Merged

5 tasks

gravitystorm mentioned this pull request Apr 1, 2020

Lint/UriRegexp misses common usecase of URI.regexp #7834

Closed

dependabot bot mentioned this pull request Mar 16, 2021

Bump rubocop from 0.47.1 to 0.50.0 LemonSpike/Xcodeproj#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new `Lint/UriRegexp` cop #4694

Add new `Lint/UriRegexp` cop #4694

koic commented Sep 2, 2017 •

edited

Loading

pocke Sep 3, 2017 •

edited

Loading

koic Sep 3, 2017 •

edited

Loading

pocke Sep 3, 2017

koic Sep 3, 2017 •

edited

Loading

pocke Sep 3, 2017

koic Sep 3, 2017

pocke left a comment

Drenmi Sep 4, 2017

koic Sep 4, 2017

Drenmi left a comment

koic Sep 5, 2017

rrosenblum commented Sep 5, 2017

rrosenblum Sep 5, 2017

koic Sep 6, 2017

koic commented Sep 6, 2017

bbatsov Sep 10, 2017

koic Sep 10, 2017

bbatsov Sep 10, 2017

koic Sep 10, 2017

Add new Lint/UriRegexp cop #4694

Add new Lint/UriRegexp cop #4694

Conversation

koic commented Sep 2, 2017 • edited Loading

Feature

Target Problem

pocke Sep 3, 2017 • edited Loading

Choose a reason for hiding this comment

koic Sep 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koic Sep 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pocke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Drenmi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrosenblum commented Sep 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koic commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add new `Lint/UriRegexp` cop #4694

Add new `Lint/UriRegexp` cop #4694

koic commented Sep 2, 2017 •

edited

Loading

pocke Sep 3, 2017 •

edited

Loading

koic Sep 3, 2017 •

edited

Loading

koic Sep 3, 2017 •

edited

Loading