-
Notifications
You must be signed in to change notification settings - Fork 252
Extract paged search result cookie handling logic #185
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
Conversation
@mynameisrufus I thought rubocop was not going to cause build failures for existing code? I think this could be a worthwhile refactoring, but I'm wary of doing a simple method extraction like this. Does this new method make sense standalone? Perhaps it does, but I think the method comment needs to be updated with a better explanation. Otherwise, we're just splitting and hiding behavior in more places and making the code harder to reason about. |
Sorry for late reply, I am on holiday. I will try and figure out what this bit of code is for and change documentation. |
"we're just splitting and hiding behavior in more places and making the code harder to reason about" or is this encapsulating behaviour and making it easier to reason about? |
@mynameisrufus I'm not saying your refactoring is hiding behavior. From your method name, it sounds like a good candidate for extraction, but the method comment should also work stand alone. The current description does not explain what a search cookie is, assumes there is a loop going on, and is generally tied to the caller. If I came across this, I would be confused:
|
I have removed Austin Ziegler's 2010 code comments (8e9a8a4) and replaced it with a slimed down version with a link to the RFC, the RFC actually explains things, no point in repeating in comment. |
👍 nice. @mtodd care to chime in? |
break unless more_pages | ||
cookie = next_search_result_cookie(result_pdu.result_code, controls) | ||
rfc2696_cookie[1] = cookie unless cookie.nil? | ||
break if cookie.nil? |
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.
Could reorder this and skip the unless cookie.nil?
:
cookie = next_search_result_cookie(result.pdu.result_code, controls)
break if cookie.nil?
rfc2696_cookie[1] = cookie
Only minor nits, this extraction looks great! Thanks @mynameisrufus! |
Extracted the rfc cookie find code in searh into its own method to fix the following offense that broke the build: ``` lib/net/ldap/connection.rb:379:3: C: Assignment Branch Condition size for search is too high. ```
@mtodd made the changes, ready for merge 🐐 |
@mynameisrufus yep, looks great! I have some time tomorrow to do some manual testing (because I don't think our existing test suite covers paged search results) before merging. |
Unblocks ruby-ldap#183. There is a proposed fix in ruby-ldap#185, but it's not ready yet.
Extracted the rfc cookie find code in search into its own method to fix the
following offense that broke the build: