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.

Sign up for GitHub

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

Jump to bottom

[stdlib] Support reversed for Dict #2340

Closed
wants to merge 19 commits into from
Closed

[stdlib] Support reversed for Dict #2340

wants to merge 19 commits into from
+152 −15

Conversation

jayzhan211
Copy link
Contributor

@jayzhan211 jayzhan211 commented Apr 19, 2024 •
edited

Part of #2325

Follow up PR on #2327

I done the values and items of reversed dict, since #2327 is not yet merged, so this PR also includes #2327.
If this PR looks good, we can merge this, otherwise #2327 can be merged beforehand

@jayzhan211
dict reverse
59cdebc 
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
@jayzhan211
upd doc
443d2e7 
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
@jayzhan211
param if
2b06ab5 
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
@jayzhan211
dict values reverse
c50323f 
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
@jayzhan211
fix test
e5e842c 
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
@jayzhan211
cleanup
1aff30a 
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
@jayzhan211
support items
12a1c74 
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
@jayzhan211
cleanup
1a100ab 
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
@jayzhan211 jayzhan211 requested a review from a team as a code owner April 19, 2024 13:11
@jayzhan211 jayzhan211 changed the title Support reversed for Dict [stdlib] Support reversed for Dict Apr 19, 2024
@JoeLoser
Copy link
Collaborator

Feel free to rebase now that abf2975 just landed for reversing the keys. Sorry I didn't see this PR before merging the smaller one.

@jayzhan211
Copy link
Contributor Author

@JoeLoser Done!

@jayzhan211
Copy link
Contributor Author

I just notice Joe is on PTO, @ConnorGray ready for review!

@JoeLoser
Copy link
Collaborator

@jayzhan211 do you mind rebasing and fixing up the merge conflicts? After that, happy to take a look. Thank you!

@jayzhan211 jayzhan211 requested review from jackos and a team as code owners April 27, 2024 23:20
@jayzhan211
Copy link
Contributor Author

@JoeLoser Sure!

@jayzhan211
Copy link
Contributor Author

@JoeLoser Friendly ping, this PR is ready for review, thanks!

@JoeLoser JoeLoser added the imported-internally Signals that a given pull request has been imported internally. label May 5, 2024
@JoeLoser
Copy link
Collaborator

JoeLoser commented May 5, 2024

@JoeLoser Friendly ping, this PR is ready for review, thanks!

Thanks for the friendly ping. I appreciate you rebasing — I just imported this PR!

JoeLoser
JoeLoser approved these changes May 5, 2024
View reviewed changes
Copy link
Collaborator

@JoeLoser JoeLoser left a comment •
edited

Choose a reason for hiding this comment

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

Looks great! Do you mind adding a changelog entry, please? I'm going to start suggesting external contributors can write their own changelog entries where it makes sense if you don't mind. You're the first one I'm asking as we make this change 😄

stdlib/src/collections/dict.mojo Show resolved Hide resolved
stdlib/test/builtin/test_reversed.mojo Outdated Show resolved Hide resolved
stdlib/test/builtin/test_reversed.mojo Show resolved Hide resolved
stdlib/test/builtin/test_reversed.mojo Show resolved Hide resolved
@jayzhan211
add changelog entry
74b3cdb 
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
@jayzhan211
upd changelog
c75c5ec 
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
@jayzhan211 jayzhan211 requested a review from JoeLoser May 5, 2024 04:48
@gabrieldemarmiesse gabrieldemarmiesse mentioned this pull request May 6, 2024
Draft
@ematejska ematejska added the mojo-repo Tag all issues with this label label May 6, 2024
@JoeLoser
Copy link
Collaborator

JoeLoser commented May 7, 2024

@jayzhan211 do you mind rebasing to fix the conflicts from today's nightly and then I'm happy to land this? Thank you!

@jayzhan211
Copy link
Contributor Author

@JoeLoser Done!

@JoeLoser JoeLoser added the merged-internally Indicates that this pull request has been merged internally label May 7, 2024
@JoeLoser
Copy link
Collaborator

JoeLoser commented May 7, 2024

✅🟣 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.

@JoeLoser JoeLoser added the merged-externally Merged externally in public mojo repo label May 8, 2024
@JoeLoser JoeLoser closed this May 8, 2024
@jayzhan211
Copy link
Contributor Author

@JoeLoser Do you know why the change is not appeared in nightly?

https://github.com/modularml/mojo/blob/nightly/stdlib/src/builtin/reversed.mojo

@JoeLoser JoeLoser removed merged-internally Indicates that this pull request has been merged internally merged-externally Merged externally in public mojo repo labels May 12, 2024
@JoeLoser JoeLoser reopened this May 12, 2024
@JoeLoser
Copy link
Collaborator

@JoeLoser Do you know why the change is not appeared in nightly?

https://github.com/modularml/mojo/blob/nightly/stdlib/src/builtin/reversed.mojo

This is my fault, I'm sorry. Before we had this piece of automation up for auto commenting upon merging internally, I made a mistake here. This one has not landed internally and so isn't reflected on the nightly branch here. I've reopened this pull request and removed the incorrect labels I added.

I'll take a look on Monday for shepherding this PR through internally so it can land. I'm sorry about the troubles here - it's completely my fault. Thank you for calling it out!

@JoeLoser
Copy link
Collaborator

@jayzhan211 okay I actually merged it internally this time just now 🎉

@JoeLoser JoeLoser added the merged-internally Indicates that this pull request has been merged internally label May 13, 2024
@patrickdoc patrickdoc closed this May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackos jackos Awaiting requested review from jackos

@JoeLoser JoeLoser Awaiting requested review from JoeLoser

Assignees
No one assigned
Labels
imported-internally Signals that a given pull request has been imported internally. merged-internally Indicates that this pull request has been merged internally mojo-repo Tag all issues with this label
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jayzhan211 @JoeLoser @ematejska