-
Notifications
You must be signed in to change notification settings - Fork 2.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
[stdlib] Implement List.__repr__() method #2634
Conversation
…_() one Signed-off-by: Manuel Saelices <msaelices@gmail.com>
Signed-off-by: Manuel Saelices <msaelices@gmail.com>
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.
Thank you for the pull request! If you remove the trait, this PR should be good to merge :)
stdlib/src/collections/list.mojo
Outdated
@@ -72,7 +72,9 @@ struct _ListIter[ | |||
return self.index | |||
|
|||
|
|||
struct List[T: CollectionElement](CollectionElement, Sized, Boolable): | |||
struct List[T: CollectionElement]( | |||
CollectionElement, Sized, Boolable, Representable |
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.
The list cannot be Representable
yet because __repr__
is a staticmethod and not a method. We need conditional conformance to make List Representable
conditionally. I think you can remove it.
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.
Done: msaelices@1c1dec9
@@ -31,6 +31,12 @@ fn test_variadic_list() raises: | |||
check_list(5, 8, 6) | |||
|
|||
|
|||
fn test_repr_list() raises: |
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.
Can you also add a test case for an empty List? That would ensure the code doesn't break in the future.
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.
Done: msaelices@f552e50
…method yet Signed-off-by: Manuel Saelices <msaelices@gmail.com>
Signed-off-by: Manuel Saelices <msaelices@gmail.com>
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.
Looks good! I think it's ready to be merged. Thank you for your contribution!
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.
Looks good, thank you! We have a trick proven out today for the List.index()
method that I think we can apply here as well when the new nightly is available later tonight/tomorrow. I'll merge this internally in the meantime and we can refactor it as a static method to avoid the __type_of
dance.
✅🟣 This contribution has been merged 🟣✅ Your pull request has been merged to the internal upstream Mojo sources. It will be reflected here in the Mojo repository on the nightly branch during the next Mojo nightly release, typically within the next 24-48 hours. We use Copybara to merge external contributions, click here to learn more. |
[External] [stdlib] Implement List.__repr__() method Based on the `List.__str__()` one, to allow to call `repr(some_list)`. It's not currently passing because of the following error, but I think the code is correct and it's a limitation of the mojo compiler, which does not understand that, if `self` is made of `RepresentableCollectionElement`, the `__repr__` method can call the `__str__` one, as `__type_of(self)` will also be `List[RepresentableCollectionElement]` ![image](https://github.com/modularml/mojo/assets/136875/b4fc0292-d2db-426e-8b61-33003e98de3d) Co-authored-by: Manuel Saelices <msaelices@gmail.com> Closes #2634 MODULAR_ORIG_COMMIT_REV_ID: b939d766211d3666fe65eea198f4bf24e734a180
Landed in ba59164! Thank you for your contribution 🎉 |
[External] [stdlib] Implement List.__repr__() method Based on the `List.__str__()` one, to allow to call `repr(some_list)`. It's not currently passing because of the following error, but I think the code is correct and it's a limitation of the mojo compiler, which does not understand that, if `self` is made of `RepresentableCollectionElement`, the `__repr__` method can call the `__str__` one, as `__type_of(self)` will also be `List[RepresentableCollectionElement]` ![image](https://github.com/modularml/mojo/assets/136875/b4fc0292-d2db-426e-8b61-33003e98de3d) Co-authored-by: Manuel Saelices <msaelices@gmail.com> Closes modularml#2634 MODULAR_ORIG_COMMIT_REV_ID: b939d766211d3666fe65eea198f4bf24e734a180 Signed-off-by: Lukas Hermann <lukashermann28@gmail.com>
[External] [stdlib] Implement List.__repr__() method Based on the `List.__str__()` one, to allow to call `repr(some_list)`. It's not currently passing because of the following error, but I think the code is correct and it's a limitation of the mojo compiler, which does not understand that, if `self` is made of `RepresentableCollectionElement`, the `__repr__` method can call the `__str__` one, as `__type_of(self)` will also be `List[RepresentableCollectionElement]` ![image](https://github.com/modularml/mojo/assets/136875/b4fc0292-d2db-426e-8b61-33003e98de3d) Co-authored-by: Manuel Saelices <msaelices@gmail.com> Closes modularml#2634 MODULAR_ORIG_COMMIT_REV_ID: b939d766211d3666fe65eea198f4bf24e734a180
[External] [stdlib] Implement List.__repr__() method Based on the `List.__str__()` one, to allow to call `repr(some_list)`. It's not currently passing because of the following error, but I think the code is correct and it's a limitation of the mojo compiler, which does not understand that, if `self` is made of `RepresentableCollectionElement`, the `__repr__` method can call the `__str__` one, as `__type_of(self)` will also be `List[RepresentableCollectionElement]` ![image](https://github.com/modularml/mojo/assets/136875/b4fc0292-d2db-426e-8b61-33003e98de3d) Co-authored-by: Manuel Saelices <msaelices@gmail.com> Closes modularml#2634 MODULAR_ORIG_COMMIT_REV_ID: b939d766211d3666fe65eea198f4bf24e734a180
Based on the
List.__str__()
one, to allow to callrepr(some_list)
.It's not currently passing because of the following error, but I think the code is correct and it's a limitation of the mojo compiler, which does not understand that, if
self
is made ofRepresentableCollectionElement
, the__repr__
method can call the__str__
one, as__type_of(self)
will also beList[RepresentableCollectionElement]