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

++= appendAssign operator (#7346) #7354

Merged
merged 1 commit into from Dec 9, 2022

Conversation

raccmonteiro
Copy link
Contributor

@raccmonteiro raccmonteiro commented Dec 5, 2022

Description

Closes #7346

Tests + Formatting

> mut a = [1 2 3]
> $a ++= [4 5 6]
> $a
[1 2 3 4 5 6]

@webbedspace
Copy link
Contributor

webbedspace commented Dec 5, 2022

Please write tests. Just copy-paste the ++ tests and tweak them if you must.

@rgwood
Copy link
Contributor

rgwood commented Dec 7, 2022

Thanks for the PR! Agreed with @webbedspace that this needs some tests.

@raccmonteiro
Copy link
Contributor Author

Added some tests.
But, I noticed this does not work

> mut a = [1 2 3]
> $a ++= [a]

results in this error:
image

This should be a valid case, right? I did not understood yet where this error is being thrown.
If someone wants to help please be free to make the changes :)

Copy link
Member

@sholderbach sholderbach left a comment

Choose a reason for hiding this comment

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

Your code and tests look good to me!

@sholderbach
Copy link
Member

I think constraining the type of a mutable variable to the one at assignment time sounds less surprising, but the question is if for this the list should be opaque with the inner type erased (list<any>).

Added some tests. But, I noticed this does not work

> mut a = [1 2 3]
> $a ++= [a]

results in this error: image

This should be a valid case, right? I did not understood yet where this error is being thrown. If someone wants to help please be free to make the changes :)

@rgwood
Copy link
Contributor

rgwood commented Dec 9, 2022

I think constraining the type of a mutable variable to the one at assignment time sounds less surprising

I agree, this seems like the right behaviour to me.

the question is if for this the list should be opaque with the inner type erased (list).

I think a should be as specific as possible by default (list<int>), but we should allow the user to opt into list<any>. Beyond the scope of this PR but it would be nice to be able to do something like mut a: list<any> = [1 2 3]

@rgwood
Copy link
Contributor

rgwood commented Dec 9, 2022

I think this could use another test checking that the case above (mut a = [1 2 3]; $a ++= [a]) returns an error. But it's not 100% necessary and this could be merged as is. Thanks @raccmonteiro!

@rgwood
Copy link
Contributor

rgwood commented Dec 9, 2022

btw, this is interesting (but not caused by this PR, no need to fix it here):

image

I haven't taken a look at the implementation but it looks like we are not handling covariance correctly.

append_assign tests

test type mismatch
@raccmonteiro
Copy link
Contributor Author

I think this could use another test checking that the case above (mut a = [1 2 3]; $a ++= [a]) returns an error. But it's not 100% necessary and this could be merged as is. Thanks @raccmonteiro!

I added this case, I didn't do it before because I thought this should not result in error.

Thank you both for explaining your thoughts

@rgwood rgwood merged commit b56ad92 into nushell:main Dec 9, 2022
@rgwood
Copy link
Contributor

rgwood commented Dec 9, 2022

Thanks for another great PR!

@raccmonteiro raccmonteiro deleted the appendassign-operator branch December 12, 2022 10:53
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.

Add ++= operator for mutable lists
4 participants