-
Notifications
You must be signed in to change notification settings - Fork 499
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
Fix part of #1587: Made some intent/bundle/saved/tags instance keys consistent. #3350
Conversation
@rt4914 , PTAL |
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.
@ayush0402 Left some comments.
Also please link the issue with this PR
@ayush0402 To fix the failing unit tests, identify the failing test file and then try to find the cause of the error. To run the tests locally, read our wiki instructions. |
Hi @ayush0402, it looks like some changes were requested on this pull request by @prayutsu. PTAL. Thanks! |
@prayutsu Assign it to me or Akshay once approved from your side. |
@ayush0402 You should not resolve conversation threads by yourself, only the person who started the thread should it, it helps in keeping track of the changes they requested. And once you make the changes as requested by the reviewer, you should reply with "Done" or "fixed" so that the reviewer is aware that you have made changes. |
I will keep that in mind for next time. I made the requested changes. @prayutsu PTAL. |
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.
@ayush0402 PTAL. It looks like you missed adding the extra blank line while undoing the changes.
All files must end with an empty new line (also called EOF).
Also please note that the issue hasn't been linked yet. You need to correct the keyword in the PR explanation -- it should be Fixes part of #issueNumber
instead of Fix part of #issueNumber
.
Thanks!
Unassigning @ayush0402 since a re-review was requested. @ayush0402, please make sure you have addressed all review comments. Thanks! |
Unassigning @prayutsu since the review is done. |
Hi @ayush0402, it looks like some changes were requested on this pull request by @prayutsu. PTAL. Thanks! |
@prayutsu PTAL. Made requested changes. |
Unassigning @ayush0402 since a re-review was requested. @ayush0402, please make sure you have addressed all review comments. Thanks! |
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.
LGTM, thanks @ayush0402
Assigning @fsharpasharp for code owner reviews. Thanks! |
@ayush0402 I have resolved all the conversations as of now, but you should always reply to the conversation threads with "Done" or "Fixed" when you commit the suggested changes so that the reviewer can resolve the conversation. |
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.
Approving for the changes in IntentFactoryShimImpl.kt
Unassigning @fsharpasharp since they have already approved the PR. |
Please follow the first point in the PR checklist and mark it did once done. |
@anandwana001 Done. PTAL. |
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.
@ayush0402 Nice work. Thanks.
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.
LGTM
Explanation
Made some keys consistent in various Activities and Fragments.
Fixes part of #1587
Checklist