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

Make sequtils.zip return seq of anonymous tuples #12575

Merged
merged 2 commits into from
Nov 4, 2019

Conversation

kaushalmodi
Copy link
Contributor

Earlier the tuples had named fields "a" and "b" and that made it
difficult to assign the zip returned seqs to other vars which expected
seqs of tuples with field names other than "a" and "b".

@kaushalmodi kaushalmodi force-pushed the make-zip-return-anon-tup-seq branch 4 times, most recently from cfb9d55 to 4cda53a Compare November 1, 2019 22:12
Copy link
Contributor

@krux02 krux02 left a comment

Choose a reason for hiding this comment

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

You should add a test that tests the behavior that you want. Test the assignment to a named tuple with some arbitrary names.

@kaushalmodi
Copy link
Contributor Author

@krux02 That's a good idea. I'll get such a test in by tomorrow.

@krux02
Copy link
Contributor

krux02 commented Nov 1, 2019

I just want to mention that I was once bothered by the field names in the tuple as well. So thumbs up for this PR.

@kaushalmodi
Copy link
Contributor Author

Thanks :) I have updated the tests.

@kaushalmodi kaushalmodi force-pushed the make-zip-return-anon-tup-seq branch 2 times, most recently from cda6efb to 0e0aa60 Compare November 2, 2019 01:00
lib/pure/collections/sequtils.nim Outdated Show resolved Hide resolved
@mratsim
Copy link
Collaborator

mratsim commented Nov 2, 2019

So anonymous tuples can be assigned to other anonymous tuples or to named tuples (i.e. structural typing) but named tuples can only be assigned to other named tuples (i.e. "string"/"identifier" typing).

I think this part is underspecified. Also while we tuple destructuring works for both anonymous and named tuple, maybe a function or magic that anonymize named tuples is needed for function call chaining like the original case?

@kaushalmodi
Copy link
Contributor Author

kaushalmodi commented Nov 2, 2019

but named tuples can only be assigned to other named tuples (i.e. "string"/"identifier" typing)

No, named tuples can be assigned to named tuples with the exact name fields, or also to any anonymous tuple.

https://scripter.co/notes/nim/#tuple-type-equality

@Varriount
Copy link
Contributor

Does this break backwards compatibility at all?

@kaushalmodi
Copy link
Contributor Author

@Varriount Yes, slightly, if someone is relying on inferred typing.

E.g. this will break:

let foo = zip(someSeq1, someSeq2)
echo foo.a

But this will not break:

let foo: seq[tuple[a:int, b:int]] = zip(someSeq1, someSeq2) # of course the types of a and b fields are assumed here
echo foo.a

@krux02
Copy link
Contributor

krux02 commented Nov 4, 2019

Good point about the breaking behavior. We do have a backwards compatibility claim. But I am still positive for this change, since I really do like it. You should introduce a backwards compatibility branch. This is how you do it:

when (NimMajor, NimMinor) <= (1, 0) # 1.0.x
  # old declaration here
else:
  # new declration here

Of course you also need to have a test for the old version (reintroduce the broken tests into another file that is compiled with --useVersion:1.0. Sorry for this inconvenience.

@kaushalmodi
Copy link
Contributor Author

OK, I'll get to this later today.

I have a question though..

Does the CI run all the tests with and without --useVersion:1.0.

@Araq
Copy link
Member

Araq commented Nov 4, 2019

Does the CI run all the tests with and without --useVersion:1.0.

No and it's worse than that, right now --useVersion:1.0 doesn't do anything (yet!).

@kaushalmodi
Copy link
Contributor Author

kaushalmodi commented Nov 4, 2019

Also, looks like --useVersion:1.0 would apply to the whole core? So if someone wants old version of zip, they would need to take the old versions of everything?

Update: I'm not staying that that is bad. I am just confirming what I understand here. It's good that people get either all the old stuff in one go or all the new better stuff. If they choose to keep their code untouched and frozen in Nim 1.0, then this wouldn't make any difference to them.

@Araq
Copy link
Member

Araq commented Nov 4, 2019

Yes, it applies to the whole core.

Earlier the tuples had named fields "a" and "b" and that made it
difficult to assign the zip returned seqs to other vars which expected
seqs of tuples with field names other than "a" and "b".
@kaushalmodi
Copy link
Contributor Author

kaushalmodi commented Nov 4, 2019

@krux02 , @Araq : Please review the new commit added to make zip backwards compatible: c0b597c


The only thing is that right now, there isn't a robust way to test out both of those when/else clauses.

@kaushalmodi
Copy link
Contributor Author

I have redone the code change to make zip backward compatible using the tip from @PMunch (https://play.nim-lang.org/#ix=20MO).

/cc @narimiran @PMunch Can you please review c0b597c ?

@Araq Araq merged commit b24560a into nim-lang:devel Nov 4, 2019
kiyolee pushed a commit to kiyolee/nim that referenced this pull request Nov 7, 2019
* Make sequtils.zip return seq of anonymous tuples

Earlier the tuples had named fields "a" and "b" and that made it
difficult to assign the zip returned seqs to other vars which expected
seqs of tuples with field names other than "a" and "b".

* Make sequtils.zip backwards compatible with Nim 1.0.x
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.

5 participants