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

UUID.[] constructor #13074

Open
straight-shoota opened this issue Feb 14, 2023 · 7 comments
Open

UUID.[] constructor #13074

straight-shoota opened this issue Feb 14, 2023 · 7 comments

Comments

@straight-shoota
Copy link
Member

#11879 changes the UUID#inspect format to code that can be used to recreate a UUID instance with the same value using the [] constructor.

This constructor however doesn't exist yet. This is a feature request to add it.
The exact semantics need to be determined.
UUID has has four different .new overloads accepting either String, StaticArray, Bytes or UUID as main parameter. I suppose it makes sense to accept the same types for .[] and just forward to the respective overload. Although for this syntax, I figure the String overload to be most useful. Perhaps we should leave out the others?
Another question is whether the optional version and variant parameters should also be supported. It might not be super necessary, but it's easy enough to do that (they're identical in all .new overloads).
I would make them only available as named parameters for clarity, though (should probably do that for .new as well).

@oprypin
Copy link
Member

oprypin commented Feb 15, 2023

This is a −1 from me.
The logic chain has gone too far.

Writing .new is annoying → let's add [] as an alternative to types where that really makes sense → let's start moving .inspect to use that because .new is annoying there too → let's add constructors just to match the decision to use [] in inspect.

I draw the line even a bit earlier - I am also against using [] in #11879 - there's just no logic to such a change.

@straight-shoota
Copy link
Member Author

straight-shoota commented Feb 15, 2023

I thought we had a achieved a broad consensus on the [] constructor pattern.

let's add [] as an alternative to types where that really makes sense

What would be types where it makes sense and where doesn't it? I don't see how such a distinction would be made.

I am also against using [] in #11879 - there's just no logic to such a change.

The main intention is for #inspect to produce a format that can be used as Crystal code and reproduces the exact same UUID value. I think this is a very useful feature.
The current format is almost there, but doesn't work as Crystal code. That's a subpar experience and just adding a small change can make it so much easier to copy & paste the inspect output into source code.

Now whether that format uses a new .[] or the existing .new constructor is a secondary question. But I think the arguments for [] are pretty solid: Avoid an explicit call syntax and being more succinct.
The only downside if you want to call it that is adding a new constructor .[] which is effectively an alias for .new. I think that's acceptable considering the overall benefits and being an instance of a recurring pattern.

@asterite
Copy link
Member

@straight-shoota This is Ruby

$ irb
irb(main):001:0> Set[1, 2, 3]
=> #<Set: {1, 2, 3}>

Look! inspect doesn't even produce an output that's parseable in Ruby for Set!

I think if there's an easy way to have inspect produce something that's valid Crystal code, go for it. This is easy to do in Crystal for Array-like and Hash-like structures because of Array-like and Hash-like literals. Then we can do the same for types that have a literal, like ints, strings, etc.

But if you have to start changing a type's API just so that inspect produces something that's valid Crystal code... that's a big NO from me. Ruby didn't do it either. There's a reason to it: it's impossible for inspect to always produce valid Ruby code.

Hey! Look at this:

$ irb
irb(main):001:0> a = []
=> []
irb(main):002:0> a << a
=> [[...]]

BAM! That's not valid Ruby anymore.

@straight-shoota
Copy link
Member Author

straight-shoota commented Feb 15, 2023

Look! inspect doesn't even produce an output that's parseable in Ruby for Set!

Why should that prevent Crystal to do better?

But if you have to start changing a type's API just so that inspect produces something that's valid Crystal code... that's a big NO from me.

Why is that a no? I don't see any reason why that would be a bad thing.
It helps adding a very convenient feature that improves developer experience.

@asterite
Copy link
Member

asterite commented Feb 15, 2023

Why is that a no? I don't see any reason why that would be a bad thing.

Crystal tries to avoid having multiple ways to do the same thing. If you have UUID.new and UUID[...] then you have two ways to do the same thing. People will need to learn those two things. People will start changing .new to [...] and discussing over that.

Maybe the question is: in which scenario you saw a UUID being inspected and you wanted to copy that output for something?

@asterite
Copy link
Member

For that matter, I don't understand why UUID needs this special treatment. What about Bigint? An Atomic value? Etc. Are we going to adjust the entire standard library for that?

@straight-shoota
Copy link
Member Author

Yes, it adds an additional way to do the same thing. And Crystal intentionally tries to avoid that. But we're not dogmatic about it. Sometimes it's worth more to have some duplication when it brings other value.

In this case it brings the great convenience of being able to copy the output of #inspect into Crystal code and recreate the same instance. That's an awesome achievement. The benefit over re-using existing .new constructors is more conciseness and avoiding explicit method call syntax (as per #11879 (comment) and #11879 (comment)). I think that's enough to justify a .[] constructor additional to .new.

It's not something that you have to explicitly learn about for every type. It's a generic pattern and you only need to know that the .[] constructor usually works the same way as .new for the most common use cases.

I don't recall the exact circumstances, but I've wished UUID would be that convenient two or three times before I felt the need to address this in #11879.

I don't understand why UUID needs this special treatment. What about Bigint? An Atomic value?

This is explicitly not a specific development about UUID, we're just discussing UUID in this particular issue.

We've adopted the general pattern already in a couple of types and expect to expand to more where it makes sense. That's usually for simple value types and maybe some container types as well..
Path.[] is already a thing. We've just recently added Process::Status#inspect (#13044), Enum.[] (#12900) and matching Enum#inspect (#13004) which affect all enum types.
I could certainly see adopting this for BigInt and other big number types.
Atomic is not a value type and there's little use for transferring the value of an instance into code
Other stdlib types that could perhaps make use of this pattern are URI, Time, Time::Span and Socket::Address.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants