phony_rails
phony_rails copied to clipboard
joost
plausible_number? is far too permissive
Because normalize_number strips all non-phonelike characters from its input, you get weird results like the following:
PhonyRails.plausible_number?('https://doi.org/10.1002/rse2.195', country_code: 'GB')
# => true
Any suggested fix? Open to pull requests.
On Thu, Jan 28, 2021, 12:19 Chris Howlett [email protected] wrote:
Because normalize_number strips all non-phonelike characters from its input, you get weird results like the following:
PhonyRails.plausible_number?('https://doi.org/10.1002/rse2.195', country_code: 'GB')
=> true
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/joost/phony_rails/issues/204, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAWKSXCSGCAALXLXJFVJDTS4FB2PANCNFSM4WW4E3AA .
Well, it depends on what you, as owner, want the behaviour to be. I see that you strip any non-(digit, bracket or plus), and I can see that stripping dashes from +44-1234-567-890 (for instance) makes sense. Letters feel like they're less likely to appear in a valid number (especially since extensions are dealt with earlier).
Perhaps it should strip all non-phonelike, non-alphabetic characters?
@joost I want to try and submit a pull request, but I can't get tests running. I've:
- Forked asilano/phony_rails
- Cloned
- Bundled
-
bundle exec rspec spec
I get the following error output:
No DRb server is running. Running in local process instead ...
[Coveralls] Set up the SimpleCov formatter.
[Coveralls] Using SimpleCov's default settings.
An error occurred while loading ./spec/lib/phony_rails_spec.rb.
Failure/Error: require 'mongoid'
NameError:
uninitialized constant ActiveModel::Serializers::Xml
# ./spec/spec_helper.rb:12:in `require'
# ./spec/spec_helper.rb:12:in `<top (required)>'
# ./spec/lib/phony_rails_spec.rb:3:in `require'
# ./spec/lib/phony_rails_spec.rb:3:in `<top (required)>'
[Coveralls] Set up the SimpleCov formatter.
[Coveralls] Using SimpleCov's default settings.
An error occurred while loading ./spec/lib/validators/phony_validator_spec.rb.
Failure/Error: require 'mongoid'
NameError:
uninitialized constant ActiveModel::Serializers::Xml
# ./spec/spec_helper.rb:12:in `require'
# ./spec/spec_helper.rb:12:in `<top (required)>'
# ./spec/lib/validators/phony_validator_spec.rb:3:in `require'
# ./spec/lib/validators/phony_validator_spec.rb:3:in `<top (required)>'
No examples found.
Finished in 0.00006 seconds (files took 1.1 seconds to load)
0 examples, 0 failures, 2 errors occurred outside of examples
[Coveralls] Outside the CI environment, not sending data.
My Gemfile.lock now looks like this:
PATH
remote: .
specs:
phony_rails (0.14.13)
activesupport (>= 3.0)
phony (> 2.15)
GEM
remote: https://rubygems.org/
specs:
activemodel (6.1.3)
activesupport (= 6.1.3)
activerecord (6.1.3)
activemodel (= 6.1.3)
activesupport (= 6.1.3)
activesupport (6.1.3)
concurrent-ruby (~> 1.0, >= 1.0.2)
i18n (>= 1.6, < 2)
minitest (>= 5.1)
tzinfo (~> 2.0)
zeitwerk (~> 2.3)
ast (2.4.2)
bson (3.2.7)
coderay (1.1.3)
concurrent-ruby (1.1.8)
connection_pool (2.2.3)
coveralls (0.8.23)
json (>= 1.8, < 3)
simplecov (~> 0.16.1)
term-ansicolor (~> 1.3)
thor (>= 0.19.4, < 2.0)
tins (~> 1.6)
diff-lcs (1.4.4)
docile (1.3.5)
ffi (1.14.2)
formatador (0.2.5)
guard (2.16.2)
formatador (>= 0.2.4)
listen (>= 2.7, < 4.0)
lumberjack (>= 1.0.12, < 2.0)
nenv (~> 0.1)
notiffany (~> 0.0)
pry (>= 0.9.12)
shellany (~> 0.0)
thor (>= 0.18.1)
guard-bundler (3.0.0)
bundler (>= 2.1, < 3)
guard (~> 2.2)
guard-compat (~> 1.1)
guard-compat (1.2.1)
guard-rspec (4.7.3)
guard (~> 2.1)
guard-compat (~> 1.1)
rspec (>= 2.99.0, < 4.0)
i18n (1.8.9)
concurrent-ruby (~> 1.0)
json (2.5.1)
listen (3.4.1)
rb-fsevent (~> 0.10, >= 0.10.3)
rb-inotify (~> 0.9, >= 0.9.10)
lumberjack (1.2.8)
method_source (1.0.0)
minitest (5.14.3)
mongoid (4.0.0.beta2)
activemodel (>= 4.0.0)
moped (~> 2.0.beta6)
origin (~> 2.1)
tzinfo (>= 0.3.37)
moped (2.0.7)
bson (~> 3.0)
connection_pool (~> 2.0)
optionable (~> 0.2.0)
nenv (0.3.0)
notiffany (0.1.3)
nenv (~> 0.1)
shellany (~> 0.0)
optionable (0.2.0)
origin (2.3.1)
parallel (1.20.1)
parser (3.0.0.0)
ast (~> 2.4.1)
phony (2.18.19)
pry (0.14.0)
coderay (~> 1.1)
method_source (~> 1.0)
rainbow (3.0.0)
rake (13.0.3)
rb-fsevent (0.10.4)
rb-inotify (0.10.1)
ffi (~> 1.0)
regexp_parser (2.0.3)
rexml (3.2.4)
rspec (3.10.0)
rspec-core (~> 3.10.0)
rspec-expectations (~> 3.10.0)
rspec-mocks (~> 3.10.0)
rspec-core (3.10.1)
rspec-support (~> 3.10.0)
rspec-expectations (3.10.1)
diff-lcs (>= 1.2.0, < 2.0)
rspec-support (~> 3.10.0)
rspec-mocks (3.10.2)
diff-lcs (>= 1.2.0, < 2.0)
rspec-support (~> 3.10.0)
rspec-support (3.10.2)
rubocop (1.10.0)
parallel (~> 1.10)
parser (>= 3.0.0.0)
rainbow (>= 2.2.2, < 4.0)
regexp_parser (>= 1.8, < 3.0)
rexml
rubocop-ast (>= 1.2.0, < 2.0)
ruby-progressbar (~> 1.7)
unicode-display_width (>= 1.4.0, < 3.0)
rubocop-ast (1.4.1)
parser (>= 2.7.1.5)
rubocop-performance (1.9.2)
rubocop (>= 0.90.0, < 2.0)
rubocop-ast (>= 0.4.0)
ruby-progressbar (1.11.0)
shellany (0.0.1)
simplecov (0.16.1)
docile (~> 1.1)
json (>= 1.8, < 3)
simplecov-html (~> 0.10.0)
simplecov-html (0.10.2)
sqlite3 (1.4.2)
sync (0.5.0)
term-ansicolor (1.7.1)
tins (~> 1.0)
thor (1.1.0)
tins (1.28.0)
sync
tzinfo (2.0.4)
concurrent-ruby (~> 1.0)
unicode-display_width (2.0.0)
zeitwerk (2.4.2)
PLATFORMS
ruby
DEPENDENCIES
activerecord (>= 3.0)
coveralls
guard
guard-bundler
guard-rspec
mongoid (>= 3.0)
phony_rails!
rake
rspec
rubocop
rubocop-performance
sqlite3 (>= 1.4.0)
BUNDLED WITH
2.1.4
Hi @asilano! I see things broke when using phony_rails with the more recent gems.
I've fixed the tests by some updates & by removing MongoDb support.
I will push a branch for you to have a look at little later today.
Please see the https://github.com/joost/phony_rails/tree/no_mongoid branch for a version with working specs. Pull request welcome!
Thanks, the branch works nicely.
Unfortunately, my fix doesn't - normalize_number is not massively to blame, and in fact Phony thinks that Phony.plausible?('https://example.com/15.55/a5b5c5/rse5.555') is true as well.
I'll have a think, and maybe I'll go bother them too :)