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 test for nested tuple access #4570

Conversation

sasurau4
Copy link
Contributor

@sasurau4 sasurau4 commented Dec 1, 2020

Helps with #4470

  • pass cargo make test

@calebcartwright
Copy link
Member

Thank you for the PR @sasurau4 adding the tests! Be sure to review the additional comments (like #4470 (comment)) on the linked issue that call out a minor code change we want to make as well

@sasurau4 sasurau4 force-pushed the test/add-test-for-nested-tuple-access branch from 32ac238 to d086db7 Compare December 2, 2020 09:55
@sasurau4
Copy link
Contributor Author

sasurau4 commented Dec 2, 2020

@calebcartwright Thanks for pointing out. I fixed!

@calebcartwright
Copy link
Member

Perfect thank you.

Changes LGTM, though would you mind doing me a favor and including the below snippets in the tests too? They'll be formatted fine here but since these uses cases were reported in #4543 I'd like to include them in the test suite to prevent any future regressions

let oid = oid!(1.2.840.113549.1.1.5);
let oid2 = oid!(2.5.4.3);

Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

👍

@sasurau4
Copy link
Contributor Author

sasurau4 commented Dec 8, 2020

@calebcartwright Thanks for your review. I added the test for issue-4543 👍

@calebcartwright
Copy link
Member

Thanks!

@calebcartwright calebcartwright merged commit 67ad7ea into rust-lang:master Dec 9, 2020
@sasurau4 sasurau4 deleted the test/add-test-for-nested-tuple-access branch December 9, 2020 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants