Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add spec for Symbol#name to be frozen? #824

Merged
merged 3 commits into from
Jan 8, 2021
Merged

Conversation

lxxxvi
Copy link
Contributor

@lxxxvi lxxxvi commented Jan 7, 2021

Hello 👋

Here's my attempt on Symbol#name from #823

Symbol#name has been added, which returns the name of the symbol
if it is named. The returned string is frozen. Feature #16150

I am looking forward to getting feedback.

@eregon
Copy link
Member

eregon commented Jan 7, 2021

Looks good, but we should also test that the returned String is always the same String, even for dynamically generated symbols like say :"foo#{3*4} and :"foo#{2*3*2}".

@lxxxvi
Copy link
Contributor Author

lxxxvi commented Jan 7, 2021

@eregon 👍 Thanks for the feedback. Added new assertions:

      :ruby.name.should == "ruby"
      :ルビー.name.should == "ルビー"
      :"ruby_#{1+2}".name.should == "ruby_3"

Not sure about the Japanese 🇯🇵 example, I thought it would be interesting to assert for UTF-8 too... 🙃 what do you think?

@eregon
Copy link
Member

eregon commented Jan 7, 2021

I meant we should test it's the same String instance for a given Symbol, so something like:

:"ruby_#{1+2}".name.should.equal?(:ruby_3.name)

Probably a good idea to test non-ASCII well as you did, yes.

@lxxxvi
Copy link
Contributor Author

lxxxvi commented Jan 8, 2021

Oh, I think get it now. We want to check if it's the same String instance, so same object_id, right? I adjusted the specs accordingly.

Also, I was wondering if it'd make sense adding a test for symbols having characters like \n in it?

:"foo\nbar"

Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for the new spec!

@eregon eregon merged commit 5f095a1 into ruby:master Jan 8, 2021
@lxxxvi lxxxvi deleted the symbol-name branch January 12, 2023 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants