search

ruby / gem_rbs_collection

A collection of RBS for gems.
MIT License
247 stars 101 forks source link

Replace every `String | Symbol` to interned keyword #510

Closed m11o closed 2 weeks ago

m11o commented 5 months ago

interned is defined as alias for Symbol | string in RBS 3.3. ref: https://github.com/ruby/rbs/pull/1469

This PR replaced every Symbol | string to interned. I found them by regexp: symbol\s?\|\s?string|string\s?\|\s?symbol.

ksss commented 5 months ago

I think we should carefully consider that this change will have a positive outcome.

cc @sampersand

m11o commented 5 months ago

@ksss I appreciate your replying.

interned is too generic a name.

I agree with you.

It should be given a name that makes sense in the context of each library.

As described in https://github.com/ruby/rbs/pull/1469 , string | Symbol is frequently used in many methods. I think it's possible to explain the context with the method name, and I find it tedious to think of an alias every time I use the String | Symbol type.

The intention of the creators of interned is not to be used publicly. I picked interned because I doubt anyone would actually used it.

I hadn't read that comment. Thank you for sharing it 🙏 However, after reading the comment, what I thought was that it's not about nobody using an alias named interned, but rather that there would be no new alias created using the word interned because the word is too generic 🤔 (Please tell me if my understanding is mistaken 😅 )

If this alias shouldn't be used publicly, it might be restricted better.

pocke commented 3 months ago

I'm not positive about applying this change because I feel interned is unclear enough to indicate String | Symbol.

I am okay with applying this change for gems maintained by other maintainers, such as yard by ksss. Feel free to ask them if the change is acceptable.

BTW, this change will be merged more smoothly if it is separated by the gems or owners.

m11o commented 2 weeks ago

@pocke Thank you for your advice. I will try splitting the PR by each gem. I will close this PR.