Suppress "warning: URI.regexp is obsolete".

[junaruga]

Maybe this PR fixes below issue.
https://bitbucket.org/ged/ruby-pg/issues/286/warning-uriregexp-is-obsolete

As a default behavior of the testing command, there are messages for the stdout and stderr like this.
This situation makes us harder to find a real warning issue like this issue.

I think we can suppress the outputted message as much as possible except "..." by RSpec as a default behavior of rake test.
The messages could be outputted to tmp_test_specs/setup.log or only $VERBOSE mode.

$ bundle exec rake test
...
Setting up test database for Basic type mapping
Creating the test DB
Warning: no type cast defined for type "name" with oid 19. Please cast this type explicitly to TEXT to be safe for future changes.
Warning: no type cast defined for type "regproc" with oid 24. Please cast this type explicitly to TEXT to be safe for future changes.
Tearing down test database
No pidfile (#<Pathname:/home/jaruga/git/ruby-pg/tmp_test_specs/data/postmaster.pid>)
Setting up test database for PG::TypeMapByOid
Creating the test DB
...

Below kind of warning message to output message to stderr could be tested like below method on RSpec 3.
https://github.com/ged/ruby-pg/blob/master/lib/pg/basic_type_mapping.rb#L304-L305

expect { my_method }.to output("my message").to_stdout
expect { my_method }.to output("my error").to_stderr

Ref: https://stackoverflow.com/questions/16507067/testing-stdout-output-in-rspec

[cbandy]

I like this approach, but I want to point out that this constant isn't an exact replacement.

irb(main):001:0> RUBY_VERSION
=> "2.6.0"

irb(main):002:0> URI.regexp == URI::ABS_URI
(irb):2: warning: URI.regexp is obsolete
=> false

irb(main):003:0> URI.regexp == URI::ABS_URI_REF
(irb):3: warning: URI.regexp is obsolete
=> true

ABS_URI anchors to the beginning and end like we do here and also allows some whitespace:

irb(main):004:0> URI::ABS_URI
=> /\A\s*
...
      \s*\z/x

I wonder if using a URI regexp is overkill for this situation. Should we simply check for a prefix of postgresql:// or postgres:// as described in the libpq docs?

[junaruga]

@cbandy thank you for your checking!

I will replace URI::ABS_URI with URI::ABS_URI_REF.

I wonder if using a URI regexp is overkill for this situation. Should we simply check for a prefix of postgresql:// or postgres:// as described in the libpq docs?

I do not understand what is the possible values of args.first in the context well.
The check does not judge wrongly a value that is previously judged as else condition?

      case args.first
      when URI, /\A#{URI::ABS_URI}\z/
        uri = URI(args.first)
        options.merge!( Hash[URI.decode_www_form( uri.query )] ) if uri.query
      when /=/
        # Option string style
        option_string = args.first.to_s
      else
        # Positional parameters
        options[CONNECT_ARGUMENT_ORDER.first.to_sym] = args.first
      end

[cbandy]

I do not understand what is the possible values of args.first in the context well.

I agree, this method is doing more than is even documented on Connection.new. The tests give a more complete explanation.

This method exists to handle Connection.new's ... overloaded ... arguments. When there is only one argument, this tries to detect if it is an entire connection string or if it is just a hostname.

The check does not judge wrongly a value that is previously judged as else condition?

I think I don't understand the question. The else portion of a case statement is executed after all of the expressions in when portions are falsey.

http://ruby-doc.org/core/doc/syntax/control_expressions_rdoc.html#label-case+Expression

[junaruga]

I think I don't understand the question. The else portion of a case statement is executed after all of the expressions in when portions are falsey.

I mean for example when a value of args.first is postgresql://apostgresql://, before this modification, the value is evaluated in the else statement. But after this PR's modification to match postgresql:// or postgres:// for the 1st when, the value postgresql://apostgresql:// is NOT evaluated in the else statement.

I was worry about this kind of case.

[junaruga]

Ah sorry, postgresql://apostgresql:// matches both before and after this PR.

irb(main):014:0> /\A#{URI.regexp}\z/.match('postgresql://apostgresql://') != nil
=> true

I meant if there is a value that matches 1st when before this PR and does not match 1st when after this PR when we fix by "postgresql:// or postgres:// matches for 1st when". I thought that could be issue.

[cbandy]

Agreed. I think that may be possible, but I would be surprised if it is a problem in practice. (For example, I don't think DNS names are allowed to have slashes.)

If someone's host is postgresql://noturl, then I'm okay requiring that they call it with keyword (hash) arguments:

PG.connect(host: 'postgresql://noturl')

I'll leave it to maintainers to decide!

[cbandy]

lgtm

[junaruga]

Let's merge!

[larskanis]

Thank you @junaruga for implementing and @cbandy for reviewing this!

[larskanis]

@junaruga would you mind to open a pull request to suppress and test for the warnings emitted by BasicTypeMap? I think your proposal is the best way to solve this issue.

[junaruga]

@junaruga would you mind to open a pull request to suppress and test for the warnings emitted by BasicTypeMap? I think your proposal is the best way to solve this issue.

@larskanis Yes, I can send the pull-request to suppress and test for the warnings!
Thanks for merging this PR.