Annotate Active Support string extensions #185

bdewater · 2023-11-04T14:00:25Z

Completes String extensions for #137

Type of Change

Add RBI for a new gem
Modify RBI for an existing gem
Other:

Changes

Gem name: Active Support
Gem version: 7.1
Gem source: https://github.com/rails/rails/tree/7-1-stable/activesupport/lib/active_support/core_ext/string
Gem API doc: https://guides.rubyonrails.org/active_support_core_extensions.html#extensions-to-string

KaanOzkan · 2023-11-06T18:57:41Z

rbi/annotations/activesupport.rbi

+  sig { params(separator: T.nilable(String), preserve_case: T.untyped, locale: T.nilable(Symbol)).returns(T.self_type) }
+  def parameterize(separator: "-", preserve_case: false, locale: nil); end
+
+  sig { params(count: T.nilable(Integer), locale: T.nilable(Symbol)).returns(T.self_type) }


Looks like pluralize method does a reassignment here to satisfy usages like 'ley'.pluralize(:es) so this won't work https://sorbet.run/#%23%20typed%3A%20true%0Aclass%20String%0A%20%20extend%20T%3A%3ASig%0A%20%20sig%20%7B%20params%28count%3A%20T.nilable%28Integer%29%2C%20locale%3A%20T.nilable%28Symbol%29%29.returns%28T.self_type%29%20%7D%0A%20%20def%20pluralize%28count%20%3D%20nil%2C%20locale%20%3D%20%3Aen%29%0A%20%20%20%20%22a%22%0A%20%20end%0Aend%0A%0A'len'.pluralize%28%3Aes%29

Doh! Thanks, will fix

KaanOzkan

Thank you so much! Just a few comments.

KaanOzkan · 2023-11-06T19:19:28Z

rbi/annotations/activesupport.rbi

+  def html_safe; end
+
+  sig { params(capitalize: T.untyped, keep_id_suffix: T.untyped).returns(T.self_type) }
+  def humanize(capitalize: true, keep_id_suffix: false); end


capitalize looks like a boolean, is there a reason it's T.untyped here? https://github.com/rails/rails/blob/7-1-stable/activesupport/lib/active_support/core_ext/string/inflections.rb#L262

Always a tough balance to strike when adding types to untyped code :)

I opted to err on the side of flexibility here since any truthy value will work here but happy to change to boolean if you want a more strict type.

I'm not familiar with its usages so if you think being flexible is good for cases where an object is supplied I'm okay with it.

My $0.02: We should enforce booleans, as it improves the clarity of the API.

It's easy enough for callers to toss in a !!x if they need.

rbi/annotations/activesupport.rbi

+  def first(limit = 1); end
+
+  sig { params(separate_class_name_and_id_with_underscore: T.untyped).returns(T.self_type) }
+  def foreign_key(separate_class_name_and_id_with_underscore = true); end


paracycle

I gave this a really quick view, will look in more detail later, but here are my preliminary comments

rbi/annotations/activesupport.rbi

bdewater · 2023-11-10T14:47:12Z

Thanks @paracycle! Addressed your comments so far :)

amomchilov · 2024-01-09T16:16:24Z

Rebased and resolved conflicts

amomchilov

Hey @bdewater, thanks for putting these together!

I'm just rebasing and making some small tweaks, and I'll merge this shortly.

amomchilov · 2024-01-09T16:34:49Z

rbi/annotations/activesupport.rbi

+  sig { returns(Module) }
+  def constantize; end


constantize can return anything, because anything can be assigned to constants:

ABC = 123 "ABC".constantize # => 123

Suggested change

sig { returns(Module) }

def constantize; end

sig { returns(T.anything) }

def constantize; end

Oh, nice catch.

amomchilov · 2024-01-09T16:38:08Z

rbi/annotations/activesupport.rbi

+  sig { params(separator: T.nilable(String), preserve_case: T.untyped, locale: T.nilable(Symbol)).returns(String) }
+  def parameterize(separator: "-", preserve_case: false, locale: nil); end


The separator can't be nil:

irb(main):014> "abc def ghi".parameterize => "abc-def-ghi" irb(main):015> "abc def ghi".parameterize(separator: nil) /Users/alex/.gem/ruby/3.3.0/gems/activesupport-7.0.8/lib/active_support/inflector/transliterate.rb:126:in `gsub!': no implicit conversion of nil into String (TypeError) parameterized_string.gsub!(/[^a-z0-9\-_]+/i, separator) ^^^^^^^^^^^^^^^^^^^^^^^^^^^

Suggested change

sig { params(separator: T.nilable(String), preserve_case: T.untyped, locale: T.nilable(Symbol)).returns(String) }

def parameterize(separator: "-", preserve_case: false, locale: nil); end

sig { params(separator: String, preserve_case: T.untyped, locale: T.nilable(Symbol)).returns(String) }

def parameterize(separator: "-", preserve_case: false, locale: nil); end

amomchilov · 2024-01-09T16:40:47Z

rbi/annotations/activesupport.rbi

+  sig { params(count: T.nilable(T.any(Integer, Symbol)), locale: T.nilable(Symbol)).returns(String) }
+  def pluralize(count = nil, locale = :en); end


Woah this is a weird sig (the implementation itself is strange, because the count can be the locale if you only pass 1 arg).

amomchilov · 2024-01-09T16:41:52Z

rbi/annotations/activesupport.rbi

+  sig { params(locale: T.nilable(Symbol)).returns(String) }
+  def singularize(locale = :en); end


The locale can't be nil:

irb(main):023> "cars".singularize => "car" irb(main):024> "cars".singularize(nil) /Users/alex/.gem/ruby/3.3.0/gems/i18n-1.14.1/lib/i18n/locale/fallbacks.rb:61:in `[]': nil is not a valid locale (I18n::InvalidLocale) old_raise.call(*args, **kwargs) ^^^^^^^^^^^^^^^

Suggested change

sig { params(locale: T.nilable(Symbol)).returns(String) }

def singularize(locale = :en); end

sig { params(locale: T.nilable(Symbol)).returns(String) }

def singularize(locale = :en); end

amomchilov · 2024-01-09T16:42:47Z

rbi/annotations/activesupport.rbi

+  sig { params(patterns: T.nilable(T.any(Regexp, String))).returns(String) }
+  def remove(*patterns); end


Can't be nil:

"abc".remove(nil) /Users/alex/.gem/ruby/3.3.0/gems/activesupport-7.0.8/lib/active_support/core_ext/string/filters.rb:42:in `gsub!': wrong argument type nil (expected Regexp) (TypeError) gsub! pattern, "" ^^^^^^^^^^^

Suggested change

sig { params(patterns: T.nilable(T.any(Regexp, String))).returns(String) }

def remove(*patterns); end

sig { params(patterns: T.any(Regexp, String)).returns(String) }

def remove(*patterns); end

Likewise for #remove!

amomchilov · 2024-01-09T16:44:55Z

rbi/annotations/activesupport.rbi

+  sig { returns(T.nilable(Date)) }
+  def to_date; end


This method raises instead of returning nil:

irb(main):027> "abc".to_date /Users/alex/.gem/ruby/3.3.0/gems/activesupport-7.0.8/lib/active_support/core_ext/string/conversions.rb:48:in `parse': invalid date (Date::Error) ::Date.parse(self, false) unless blank? ^^^^^^^^^^^

Suggested change

sig { returns(T.nilable(Date)) }

def to_date; end

sig { returns(Date) }

def to_date; end

amomchilov · 2024-01-09T16:45:24Z

rbi/annotations/activesupport.rbi

+  sig { returns(T.nilable(DateTime)) }
+  def to_datetime; end


This method raises instead of returning nil:

irb(main):028> "abc".to_datetime /Users/alex/.gem/ruby/3.3.0/gems/activesupport-7.0.8/lib/active_support/core_ext/string/conversions.rb:58:in `parse': invalid date (Date::Error) ::DateTime.parse(self, false) unless blank? ^^^^^^^^^^^

Suggested change

sig { returns(T.nilable(DateTime)) }

def to_datetime; end

sig { returns(Date) }

def to_datetime; end

amomchilov · 2024-01-09T16:47:10Z

rbi/annotations/activesupport.rbi

+  sig { params(form: T.nilable(Symbol)).returns(T.nilable(Time)) }
+  def to_time(form = :local); end


Interestingly, this one does return nil on error, unlike #to_date and #to_datetime.

"abc".to_time # => nil

amomchilov · 2024-01-09T16:52:11Z

rbi/annotations/activesupport.rbi

+  def html_safe; end
+
+  sig { params(capitalize: T.untyped, keep_id_suffix: T.untyped).returns(T.self_type) }
+  def humanize(capitalize: true, keep_id_suffix: false); end


My $0.02: We should enforce booleans, as it improves the clarity of the API.

It's easy enough for callers to toss in a !!x if they need.

KaanOzkan

I'd selfishly like it if we returned a union e.g.T.any(Date, Date::Error) to signify the exception possibility but I can imagine dealing with this would be unpopular among Ruby devs.

andyw8 · 2024-01-09T18:53:44Z

@KaanOzkan do you mean you'd (in theory) like Rails to return an error rather than raise? Or is this actually the valid Sorbet way to write that a method may raise?

KaanOzkan · 2024-01-09T19:06:23Z

Yes the method will have the return instead of raise to make that work which makes what I said even less feasible. Example: https://blog.jez.io/union-types-checked-exceptions/#:~:text=Here%E2%80%99s%20how%20we%E2%80%99d%20write%20that%20in%20Sorbet%3A

bdewater · 2024-01-09T20:35:35Z

Thanks for getting it over the finish line @amomchilov !

bdewater requested a review from a team as a code owner November 4, 2023 14:00

bdewater requested review from KaanOzkan and vinistock November 4, 2023 14:00

bdewater marked this pull request as draft November 4, 2023 14:03

bdewater force-pushed the as-string branch from 6e75a37 to 932a525 Compare November 4, 2023 15:55

bdewater marked this pull request as ready for review November 4, 2023 15:55

KaanOzkan reviewed Nov 6, 2023

View reviewed changes

andyw8 mentioned this pull request Nov 9, 2023

Add signatures for all Active Support extension methods on built-in types #137

Open

24 tasks

bdewater force-pushed the as-string branch from 932a525 to 4914b89 Compare November 9, 2023 23:01

paracycle reviewed Nov 10, 2023

View reviewed changes

rbi/annotations/activesupport.rbi Outdated Show resolved Hide resolved

rbi/annotations/activesupport.rbi Outdated Show resolved Hide resolved

rbi/annotations/activesupport.rbi Outdated Show resolved Hide resolved

bdewater force-pushed the as-string branch from 4914b89 to e2f94aa Compare November 10, 2023 14:46

KaanOzkan approved these changes Nov 23, 2023

View reviewed changes

bdewater force-pushed the as-string branch from e2f94aa to cbe16af Compare December 18, 2023 16:57

Annotate Active Support string extensions

8ba8133

amomchilov force-pushed the as-string branch from cbe16af to 8ba8133 Compare January 9, 2024 16:16

KaanOzkan mentioned this pull request Jan 9, 2024

Revert "Add RBIs for all ActiveSupport String extensions" #215

Merged

amomchilov added 3 commits January 9, 2024 12:02

Fix/Improve ActiveSupport String sigs

565db4a

Prefer T::Boolean params over T.untyped

a0eb6ba

Annotate remaining String APIs

8b67e00

amomchilov approved these changes Jan 9, 2024

View reviewed changes

amomchilov mentioned this pull request Jan 9, 2024

Add RBIs for all ActiveSupport String extensions #214

Merged

3 tasks

KaanOzkan approved these changes Jan 9, 2024

View reviewed changes

amomchilov merged commit 5ed51f0 into Shopify:main Jan 9, 2024
3 checks passed

bdewater deleted the as-string branch January 9, 2024 20:35

Morriar added the rbi Change related to RBI annotations label Mar 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Annotate Active Support string extensions #185

Annotate Active Support string extensions #185

bdewater commented Nov 4, 2023

KaanOzkan Nov 6, 2023

bdewater Nov 6, 2023

KaanOzkan left a comment

KaanOzkan Nov 6, 2023

bdewater Nov 6, 2023

KaanOzkan Nov 9, 2023

amomchilov Jan 9, 2024

This comment was marked as resolved.

paracycle left a comment

bdewater commented Nov 10, 2023

amomchilov commented Jan 9, 2024

amomchilov left a comment

amomchilov Jan 9, 2024

paracycle Jan 9, 2024

amomchilov Jan 9, 2024

amomchilov Jan 9, 2024

amomchilov Jan 9, 2024

amomchilov Jan 9, 2024

amomchilov Jan 9, 2024

amomchilov Jan 9, 2024

amomchilov Jan 9, 2024

amomchilov Jan 9, 2024

KaanOzkan left a comment •

edited

Loading

andyw8 commented Jan 9, 2024 •

edited

Loading

KaanOzkan commented Jan 9, 2024

bdewater commented Jan 9, 2024

		sig { params(separator: T.nilable(String), preserve_case: T.untyped, locale: T.nilable(Symbol)).returns(String) }
		def parameterize(separator: "-", preserve_case: false, locale: nil); end

		sig { params(count: T.nilable(T.any(Integer, Symbol)), locale: T.nilable(Symbol)).returns(String) }
		def pluralize(count = nil, locale = :en); end

		sig { params(locale: T.nilable(Symbol)).returns(String) }
		def singularize(locale = :en); end

		sig { params(patterns: T.nilable(T.any(Regexp, String))).returns(String) }
		def remove(*patterns); end

		sig { params(form: T.nilable(Symbol)).returns(T.nilable(Time)) }
		def to_time(form = :local); end

Annotate Active Support string extensions #185

Annotate Active Support string extensions #185

Conversation

bdewater commented Nov 4, 2023

Type of Change

Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KaanOzkan left a comment

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

Choose a reason for hiding this comment

This comment was marked as resolved.

paracycle left a comment

Choose a reason for hiding this comment

bdewater commented Nov 10, 2023

amomchilov commented Jan 9, 2024

amomchilov left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KaanOzkan left a comment • edited Loading

Choose a reason for hiding this comment

andyw8 commented Jan 9, 2024 • edited Loading

KaanOzkan commented Jan 9, 2024

bdewater commented Jan 9, 2024

KaanOzkan left a comment •

edited

Loading

andyw8 commented Jan 9, 2024 •

edited

Loading