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 system.$(HSlice) and fix #7898 #8670

Merged
merged 2 commits into from
Dec 21, 2018
Merged

Add system.$(HSlice) and fix #7898 #8670

merged 2 commits into from
Dec 21, 2018

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Aug 16, 2018

No description provided.

@kaushalmodi
Copy link
Contributor

Can you please add spaces on both sides of ..? It looks cleaner in my opinion.

So $(1 .. 5) will output "1 .. 5".

Copy link
Collaborator

@mratsim mratsim left a 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)"
Copy link
Collaborator

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)

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Member

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. :-)

Copy link
Member

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)

Copy link
Member

@timotheecour timotheecour Dec 21, 2018

Choose a reason for hiding this comment

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

=> PR #10070

@Araq Araq merged commit da80992 into nim-lang:devel Dec 21, 2018
@metagn metagn deleted the patch-4 branch December 21, 2018 19:15
timotheecour added a commit that referenced this pull request Jan 9, 2019
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants