-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 system.$(HSlice) and fix #7898 #8670
Conversation
Can you please add spaces on both sides of So |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm skeptical about having $(23, 45)
print (Field0: 23, Field1: 45)
That leaks implementation details (that unnamed field are named Field0, Field1), and is just ugly.
So was the example showing what was intended or was it just wrong?
Note: I think the $
for HSlice should have been in a separate PR as that would avoid blocking it for unrelated discussion on $tuple
@@ -2477,7 +2477,7 @@ proc `$`*[T: tuple|object](x: T): string = | |||
## of `x`. Example: | |||
## | |||
## .. code-block:: nim | |||
## $(23, 45) == "(23, 45)" | |||
## $(23, 45) == "(Field0: 23, Field1: 45)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't the proper fix be something like:
func `$`(x: tuple): string =
result = "("
for idx, val in fieldPairs(x):
if idx != 0:
result.add ", "
result.add $val
result.add ')'
(untested)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would lose information for named tuples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO the optional names in tuples are aliases to ease writing and reading code, the tuple model however remains as an ordered list of N values. So for creating a string representation I would ignore the names and just dump the values. Another thing would be for debugging (repr
), in which case it should output the field names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@drslump please, consider ignoring them only when they start with Field0: this way you'll have a nice repr for both kinds of tuples 98% of the time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking is that $
means to string, it should not leak implementation details.
Has it been considered to introduce a $$
operator convention to generate a print-debug oriented to-string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@drslump nope there is no $$
operator. The $
operator is already for print-debug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
marshal.nim
has a $$
operator. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please, consider ignoring them only when they start with Field0
isNamedTuple
(see my suggested implementation here #10010 (comment)) is more robust than checking if they start with Field0
as it relies on compiles(t.Field0)
, which would be false for unnamed tuple even if it has a field "Field0" (and also more efficient, since only 1st field needs to be checked)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
=> PR #10070
…ld1: 2) which leaked implementation detail (#10070) * add `isNamedTuple`; make $(1, 2) be (1, 2) instead of leaking implementation detail (Field0: 1, Field1: 2) fixes this: #8670 (comment) /cc @alehander42 @Vindaar @mratsim * Note: isNamedTuple is useful in other places, eg #10010 (comment) * move isNamedTuple to helpers.nim to avoid exposing new symbol to system.nim * remove workaround in tests/vm/tissues.nim failing test now that #10218 was makes it work
No description provided.