Add FindItem operation #64
Conversation
eleanxr
left a comment
There was a problem hiding this comment.
Thank you so much for your contribution! This looks like a great start. I saw a couple of issues, I have some suggestions, and I have a couple of questions about filling out the implementation a bit further before we can merge this. Thanks again!
eleanxr
left a comment
There was a problem hiding this comment.
Forgot to set the status on my review. I'd like the issues I identified to be addressed before this can move forward.
|
Switched to a draft to better communicate when I'm ready to another review. Thank you @eleanxr for the feedback. I'll take a look at your comments and begin resolving them. |
ce0d140 to
014c433
Compare
|
@eleanxr I think I resolved everything you had requested. All tests pass. Please let me know if there is anything missing! |
jtracey
left a comment
There was a problem hiding this comment.
Looks like cargo doc also caught some minor issues.
|
I fixed the rebase gone bad, verified that line endings were correct, ran I want to say a big thank you to the both of you for being so patient with me. This is my first ever open source contribution so I'm trying my best to learn the ropes of working with other people on code. You both have been very helpful and encouraging. Hopefully, I haven't been driving you guys crazy with messages and failed attempts at a merge request. |
jtracey
left a comment
There was a problem hiding this comment.
I fixed the rebase gone bad, verified that line endings were correct, ran
cargo doc, and movedRootFolderinto thefind_item.rsfor the time being.
Thanks!
I want to say a big thank you to the both of you for being so patient with me. This is my first ever open source contribution so I'm trying my best to learn the ropes of working with other people on code.
Oh, congrats then!
You both have been very helpful and encouraging. Hopefully, I haven't been driving you guys crazy with messages and failed attempts at a merge request.
Not at all, this is pretty standard for a first submission to a project, especially a change this size. Thank you for sticking with it, hopefully shouldn't be too much longer now. :)
|
@eleanxr says she's good with this, so I'm going to go ahead and merge |
Includes the following PRs: thunderbird/ews-rs#64 thunderbird/ews-rs#73 thunderbird/ews-rs#74 thunderbird/ews-rs#75 thunderbird/ews-rs#76 Differential Revision: https://phabricator.services.mozilla.com/D279079 --HG-- extra : amend_source : 258ec005dd0839e6bee9e23045f0c605913bedad
Includes the following PRs: thunderbird/ews-rs#64 thunderbird/ews-rs#73 thunderbird/ews-rs#74 thunderbird/ews-rs#75 thunderbird/ews-rs#76 Differential Revision: https://phabricator.services.mozilla.com/D279079
I think I properly implemented this. Please let me know if it is helpful.
I also opened a bug in Bugzilla for this feature: [ews-rs] Missing FindItem request implementation in ews-rs library
Feel free to edit as you see fit, this is my first attempt at doing something like this. Also if you don't want this feature in the library, that is fine by me, just thought it might be helpful.