From 6a4395f46643ad73732148c254ecb719e7e8744a Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Fri, 29 Nov 2019 17:19:51 +0900 Subject: [PATCH] Deprecate `starts_with?` and `ends_with?` for String core extensions In the past, we sometimes hit missing `Symbol#start_with?` and `Symbol#end_with?`. https://github.com/rails/rails/commit/63256bc5d7dd77b2cce82df46c53249dab2dc2a8 https://github.com/rails/rails/commit/a8e812964d711fa03843e76ae50f5ff81cdc9e00 So I proposed `Symbol#start_with?` and `Symbol#end_with?` to allow duck typing that methods for String and Symbol, then now it is available in Ruby 2.7. https://bugs.ruby-lang.org/issues/16348 Using `String#starts_with?` and `String#ends_with?` could not be gained that conveniency, so it is preferable to not use these in the future. --- actionmailer/test/asset_host_test.rb | 2 +- actionpack/lib/action_dispatch/http/cache.rb | 2 +- actionpack/lib/action_dispatch/http/mime_type.rb | 11 +++++------ .../lib/action_view/helpers/asset_url_helper.rb | 4 ++-- .../lib/action_view/helpers/form_tag_helper.rb | 2 +- .../sqlite3/schema_statements.rb | 2 +- .../connection_adapters/sqlite3_adapter.rb | 2 +- activerecord/lib/active_record/test_fixtures.rb | 2 +- activestorage/test/models/blob_test.rb | 6 +++--- activesupport/CHANGELOG.md | 5 +++++ .../core_ext/string/starts_ends_with.rb | 2 ++ activesupport/lib/active_support/dependencies.rb | 9 ++++----- activesupport/test/core_ext/string_ext_test.rb | 14 +++++++------- guides/source/active_record_validations.md | 2 +- guides/source/active_support_core_extensions.md | 11 ----------- railties/test/application/configuration_test.rb | 11 +++++------ 16 files changed, 40 insertions(+), 47 deletions(-) diff --git a/actionmailer/test/asset_host_test.rb b/actionmailer/test/asset_host_test.rb index 9cd8cae88c..7e0d7e7cc5 100644 --- a/actionmailer/test/asset_host_test.rb +++ b/actionmailer/test/asset_host_test.rb @@ -29,7 +29,7 @@ class AssetHostTest < ActionMailer::TestCase def test_asset_host_as_one_argument_proc AssetHostMailer.config.asset_host = Proc.new { |source| - if source.starts_with?("/images") + if source.start_with?("/images") "http://images.example.com" end } diff --git a/actionpack/lib/action_dispatch/http/cache.rb b/actionpack/lib/action_dispatch/http/cache.rb index 7175e2db4b..eda0eef052 100644 --- a/actionpack/lib/action_dispatch/http/cache.rb +++ b/actionpack/lib/action_dispatch/http/cache.rb @@ -114,7 +114,7 @@ module ActionDispatch # True if an ETag is set and it's a weak validator (preceded with W/) def weak_etag? - etag? && etag.starts_with?('W/"') + etag? && etag.start_with?('W/"') end # True if an ETag is set and it isn't a weak validator (not preceded with W/) diff --git a/actionpack/lib/action_dispatch/http/mime_type.rb b/actionpack/lib/action_dispatch/http/mime_type.rb index 2ea0b93c82..5f1410dad6 100644 --- a/actionpack/lib/action_dispatch/http/mime_type.rb +++ b/actionpack/lib/action_dispatch/http/mime_type.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true require "singleton" -require "active_support/core_ext/string/starts_ends_with" module Mime class Mimes @@ -117,7 +116,7 @@ module Mime type = list[idx] break if type.q < app_xml.q - if type.name.ends_with? "+xml" + if type.name.end_with? "+xml" list[app_xml_idx], list[idx] = list[idx], app_xml app_xml_idx = idx end @@ -306,7 +305,7 @@ module Mime def to_a; end def method_missing(method, *args) - if method.to_s.ends_with? "?" + if method.to_s.end_with? "?" method[0..-2].downcase.to_sym == to_sym else super @@ -314,7 +313,7 @@ module Mime end def respond_to_missing?(method, include_private = false) - (method.to_s.ends_with? "?") || super + (method.to_s.end_with? "?") || super end end @@ -349,11 +348,11 @@ module Mime private def respond_to_missing?(method, _) - method.to_s.ends_with? "?" + method.to_s.end_with? "?" end def method_missing(method, *args) - false if method.to_s.ends_with? "?" + false if method.to_s.end_with? "?" end end end diff --git a/actionview/lib/action_view/helpers/asset_url_helper.rb b/actionview/lib/action_view/helpers/asset_url_helper.rb index b1f86059c7..8ab5fb20d3 100644 --- a/actionview/lib/action_view/helpers/asset_url_helper.rb +++ b/actionview/lib/action_view/helpers/asset_url_helper.rb @@ -80,7 +80,7 @@ module ActionView # absolute path of the asset, for example "/assets/rails.png". # # ActionController::Base.asset_host = Proc.new { |source| - # if source.ends_with?('.css') + # if source.end_with?('.css') # "http://stylesheets.example.com" # else # "http://assets.example.com" @@ -206,7 +206,7 @@ module ActionView relative_url_root = defined?(config.relative_url_root) && config.relative_url_root if relative_url_root - source = File.join(relative_url_root, source) unless source.starts_with?("#{relative_url_root}/") + source = File.join(relative_url_root, source) unless source.start_with?("#{relative_url_root}/") end if host = compute_asset_host(source, options) diff --git a/actionview/lib/action_view/helpers/form_tag_helper.rb b/actionview/lib/action_view/helpers/form_tag_helper.rb index 244b341b07..992bcd0c4c 100644 --- a/actionview/lib/action_view/helpers/form_tag_helper.rb +++ b/actionview/lib/action_view/helpers/form_tag_helper.rb @@ -134,7 +134,7 @@ module ActionView # # def select_tag(name, option_tags = nil, options = {}) option_tags ||= "" - html_name = (options[:multiple] == true && !name.to_s.ends_with?("[]")) ? "#{name}[]" : name + html_name = (options[:multiple] == true && !name.to_s.end_with?("[]")) ? "#{name}[]" : name if options.include?(:include_blank) include_blank = options[:include_blank] diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/sqlite3/schema_statements.rb index ad7c0ce5bd..50cc619181 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite3/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite3/schema_statements.rb @@ -9,7 +9,7 @@ module ActiveRecord exec_query("PRAGMA index_list(#{quote_table_name(table_name)})", "SCHEMA").map do |row| # Indexes SQLite creates implicitly for internal use start with "sqlite_". # See https://www.sqlite.org/fileformat2.html#intschema - next if row["name"].starts_with?("sqlite_") + next if row["name"].start_with?("sqlite_") index_sql = query_value(<<~SQL, "SCHEMA") SELECT sql diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb index bb9da00621..b2548dae9a 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb @@ -26,7 +26,7 @@ module ActiveRecord # Allow database path relative to Rails.root, but only if the database # path is not the special path that tells sqlite to build a database only # in memory. - if ":memory:" != config[:database] && !config[:database].to_s.starts_with?("file:") + if ":memory:" != config[:database] && !config[:database].to_s.start_with?("file:") config[:database] = File.expand_path(config[:database], Rails.root) if defined?(Rails.root) dirname = File.dirname(config[:database]) Dir.mkdir(dirname) unless File.directory?(dirname) diff --git a/activerecord/lib/active_record/test_fixtures.rb b/activerecord/lib/active_record/test_fixtures.rb index ad05c793a5..9b21dfaf65 100644 --- a/activerecord/lib/active_record/test_fixtures.rb +++ b/activerecord/lib/active_record/test_fixtures.rb @@ -44,7 +44,7 @@ module ActiveRecord if fixture_set_names.first == :all raise StandardError, "No fixture path found. Please set `#{self}.fixture_path`." if fixture_path.blank? fixture_set_names = Dir[::File.join(fixture_path, "{**,*}/*.{yml}")].uniq - fixture_set_names.reject! { |f| f.starts_with?(file_fixture_path.to_s) } if defined?(file_fixture_path) && file_fixture_path + fixture_set_names.reject! { |f| f.start_with?(file_fixture_path.to_s) } if defined?(file_fixture_path) && file_fixture_path fixture_set_names.map! { |f| f[fixture_path.to_s.size..-5].delete_prefix("/") } else fixture_set_names = fixture_set_names.flatten.map(&:to_s) diff --git a/activestorage/test/models/blob_test.rb b/activestorage/test/models/blob_test.rb index 2f2dc64f85..532896cb27 100644 --- a/activestorage/test/models/blob_test.rb +++ b/activestorage/test/models/blob_test.rb @@ -139,8 +139,8 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase blob.open do |file| assert file.binmode? assert_equal 0, file.pos - assert File.basename(file.path).starts_with?("ActiveStorage-#{blob.id}-") - assert file.path.ends_with?(".jpg") + assert File.basename(file.path).start_with?("ActiveStorage-#{blob.id}-") + assert file.path.end_with?(".jpg") assert_equal file_fixture("racecar.jpg").binread, file.read, "Expected downloaded file to match fixture file" end end @@ -161,7 +161,7 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase assert file.binmode? assert_equal 0, file.pos assert_match(/\.jpg\z/, file.path) - assert file.path.starts_with?(tmpdir) + assert file.path.start_with?(tmpdir) assert_equal file_fixture("racecar.jpg").binread, file.read, "Expected downloaded file to match fixture file" end end diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md index 4c11cfada5..75cf0562c5 100644 --- a/activesupport/CHANGELOG.md +++ b/activesupport/CHANGELOG.md @@ -1,3 +1,8 @@ +* Deprecate `starts_with?` and `ends_with?` for String core extensions. + Use the native `start_with?` and `end_with?` instead. + + *Ryuta Kamizono* + * Add override of unary plus for `ActiveSupport::Duration`. `+ 1.second` is now identical to `+1.second` to prevent errors diff --git a/activesupport/lib/active_support/core_ext/string/starts_ends_with.rb b/activesupport/lib/active_support/core_ext/string/starts_ends_with.rb index 919eb7a573..97ec1bfad6 100644 --- a/activesupport/lib/active_support/core_ext/string/starts_ends_with.rb +++ b/activesupport/lib/active_support/core_ext/string/starts_ends_with.rb @@ -3,4 +3,6 @@ class String alias_method :starts_with?, :start_with? alias_method :ends_with?, :end_with? + deprecate starts_with?: :start_with? + deprecate ends_with?: :end_with? end diff --git a/activesupport/lib/active_support/dependencies.rb b/activesupport/lib/active_support/dependencies.rb index 6d71a67a46..9a29324071 100644 --- a/activesupport/lib/active_support/dependencies.rb +++ b/activesupport/lib/active_support/dependencies.rb @@ -12,7 +12,6 @@ require "active_support/core_ext/object/blank" require "active_support/core_ext/kernel/reporting" require "active_support/core_ext/load_error" require "active_support/core_ext/name_error" -require "active_support/core_ext/string/starts_ends_with" require "active_support/dependencies/interlock" require "active_support/inflector" @@ -453,7 +452,7 @@ module ActiveSupport #:nodoc: # Search for a file in autoload_paths matching the provided suffix. def search_for_file(path_suffix) - path_suffix += ".rb" unless path_suffix.ends_with?(".rb") + path_suffix += ".rb" unless path_suffix.end_with?(".rb") autoload_paths.each do |root| path = File.join(root, path_suffix) @@ -473,9 +472,9 @@ module ActiveSupport #:nodoc: end def load_once_path?(path) - # to_s works around a ruby issue where String#starts_with?(Pathname) + # to_s works around a ruby issue where String#start_with?(Pathname) # will raise a TypeError: no implicit conversion of Pathname into String - autoload_once_paths.any? { |base| path.starts_with? base.to_s } + autoload_once_paths.any? { |base| path.start_with?(base.to_s) } end # Attempt to autoload the provided module name by searching for a directory @@ -585,7 +584,7 @@ module ActiveSupport #:nodoc: end name_error = NameError.new("uninitialized constant #{qualified_name}", const_name) - name_error.set_backtrace(caller.reject { |l| l.starts_with? __FILE__ }) + name_error.set_backtrace(caller.reject { |l| l.start_with?(__FILE__) }) raise name_error end diff --git a/activesupport/test/core_ext/string_ext_test.rb b/activesupport/test/core_ext/string_ext_test.rb index f27a4423d3..723a50af47 100644 --- a/activesupport/test/core_ext/string_ext_test.rb +++ b/activesupport/test/core_ext/string_ext_test.rb @@ -239,15 +239,15 @@ class StringInflectionsTest < ActiveSupport::TestCase assert_equal 97, "abc".ord end - def test_starts_ends_with_alias + def test_deprecated_starts_ends_with_alias s = "hello" - assert s.starts_with?("h") - assert s.starts_with?("hel") - assert_not s.starts_with?("el") + assert_deprecated { assert s.starts_with?("h") } + assert_deprecated { assert s.starts_with?("hel") } + assert_deprecated { assert_not s.starts_with?("el") } - assert s.ends_with?("o") - assert s.ends_with?("lo") - assert_not s.ends_with?("el") + assert_deprecated { assert s.ends_with?("o") } + assert_deprecated { assert s.ends_with?("lo") } + assert_deprecated { assert_not s.ends_with?("el") } end def test_string_squish diff --git a/guides/source/active_record_validations.md b/guides/source/active_record_validations.md index c65e960662..0ca15d0e56 100644 --- a/guides/source/active_record_validations.md +++ b/guides/source/active_record_validations.md @@ -985,7 +985,7 @@ and performs the validation on it. The custom validator is called using the ```ruby class MyValidator < ActiveModel::Validator def validate(record) - unless record.name.starts_with? 'X' + unless record.name.start_with? 'X' record.errors.add :name, "Need a name starting with X please!" end end diff --git a/guides/source/active_support_core_extensions.md b/guides/source/active_support_core_extensions.md index 0c0163fbee..561701c532 100644 --- a/guides/source/active_support_core_extensions.md +++ b/guides/source/active_support_core_extensions.md @@ -1212,17 +1212,6 @@ The `inquiry` method converts a string into a `StringInquirer` object making equ NOTE: Defined in `active_support/core_ext/string/inquiry.rb`. -### `starts_with?` and `ends_with?` - -Active Support defines 3rd person aliases of `String#start_with?` and `String#end_with?`: - -```ruby -"foo".starts_with?("f") # => true -"foo".ends_with?("o") # => true -``` - -NOTE: Defined in `active_support/core_ext/string/starts_ends_with.rb`. - ### `strip_heredoc` The method `strip_heredoc` strips indentation in heredocs. diff --git a/railties/test/application/configuration_test.rb b/railties/test/application/configuration_test.rb index ec5eee2ecc..03e38c8e5b 100644 --- a/railties/test/application/configuration_test.rb +++ b/railties/test/application/configuration_test.rb @@ -4,7 +4,6 @@ require "isolation/abstract_unit" require "rack/test" require "env_helpers" require "set" -require "active_support/core_ext/string/starts_ends_with" class ::MyMailInterceptor def self.delivering_email(email); email; end @@ -1710,8 +1709,8 @@ module ApplicationTests test "autoload paths do not include asset paths" do app "development" ActiveSupport::Dependencies.autoload_paths.each do |path| - assert_not_operator path, :ends_with?, "app/assets" - assert_not_operator path, :ends_with?, "app/javascript" + assert_not_operator path, :end_with?, "app/assets" + assert_not_operator path, :end_with?, "app/javascript" end end @@ -1722,8 +1721,8 @@ module ApplicationTests app "development" ActiveSupport::Dependencies.autoload_paths.each do |path| - assert_not_operator path, :ends_with?, "app/assets" - assert_not_operator path, :ends_with?, "app/webpack" + assert_not_operator path, :end_with?, "app/assets" + assert_not_operator path, :end_with?, "app/webpack" end end @@ -1733,7 +1732,7 @@ module ApplicationTests # Action Mailer modifies AS::Dependencies.autoload_paths in-place. autoload_paths = ActiveSupport::Dependencies.autoload_paths autoload_paths_from_app_and_engines = autoload_paths.reject do |path| - path.ends_with?("mailers/previews") + path.end_with?("mailers/previews") end assert_equal true, Rails.configuration.add_autoload_paths_to_load_path assert_empty autoload_paths_from_app_and_engines - $LOAD_PATH