judges-action#1034: fix Fbe::FakeOctokit, make Fbe.octo#with_disable_auto_paginate#309
judges-action#1034: fix Fbe::FakeOctokit, make Fbe.octo#with_disable_auto_paginate#309rultor merged 3 commits intozerocracy:masterfrom
Fbe::FakeOctokit, make Fbe.octo#with_disable_auto_paginate#309Conversation
…le_auto_paginate`
WalkthroughAdds a block-based helper to temporarily disable Octokit Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant O as Octo Wrapper
participant K as Underlying Octokit
rect rgba(230,240,255,0.5)
note over O: with_disable_auto_paginate (normal)
C->>O: with_disable_auto_paginate { block }
O->>O: save original = K.auto_paginate
O->>K: set auto_paginate = false
activate C
C-->>O: execute block (e.g., list_issues)
deactivate C
O->>K: restore auto_paginate = original
O-->>C: return block result
end
rect rgba(255,235,230,0.5)
note over O: with_disable_auto_paginate (exception path)
C->>O: with_disable_auto_paginate { raise }
O->>O: save original
O->>K: set auto_paginate = false
C--xO: exception
O->>K: restore auto_paginate = original
O--xC: re-raise
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
lib/fbe/octo.rb (3)
263-266: FakeOctokit: track auto_paginate state to mirror real clientNo-op accessors return
nil, which is surprising and can break checks. Keep internal state with a sensible default.- def auto_paginate=(_); end + def auto_paginate=(value) + @_auto_paginate = !!value + end - - def auto_paginate; end + def auto_paginate + @_auto_paginate.nil? ? true : @_auto_paginate + endIf you prefer explicit initialization, I can add an
initializesetting@_auto_paginate = true.
604-605: Minor fixture inconsistency: owner/html URLs still based on inputnamewhen remapping to 'foo/bazz'When
name == 810,full_namebecomes 'foo/bazz' butowner[:login],html_url, andurlstill usename(→ '810'). Consider computing these fromfull_nameto avoid impossible combinations in downstream assertions.
682-717: Align issue(144) shape with other cases (optional)144 lacks
stateandupdated_atunlike 142/143. If callers pattern‑match on these keys, add them for parity.when 144 { id: 657, number:, repo: { full_name: repo }, user: { login: 'yegor256', id: 526_301, type: 'User' }, pull_request: { merged_at: nil }, - created_at: Time.parse('2025-05-29 17:00:55 UTC') + state: 'open', + created_at: Time.parse('2025-05-29 17:00:55 UTC'), + updated_at: Time.parse('2025-05-29 17:00:55 UTC') }test/fbe/test_octo.rb (1)
443-465: Strengthen semantics: assert the yielded object is the decorated wrapperOnce
with_disable_auto_paginateyieldsself(not@origin), add an assertion to lock behavior and prevent bypassing decorators.- issues = - o.with_disable_auto_paginate do + issues = + o.with_disable_auto_paginate do |client| + assert_same(o, client) refute(o.auto_paginate) o.list_issues('foo/bar') end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/fbe/octo.rb(7 hunks)test/fbe/test_octo.rb(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
test/fbe/test_octo.rb (1)
lib/fbe/octo.rb (8)
o(275-277)repository(600-640)octo(41-219)list_issues(664-671)issue(681-730)auto_paginate(263-263)auto_paginate(265-265)with_disable_auto_paginate(202-209)
lib/fbe/octo.rb (1)
lib/fbe/issue.rb (1)
issue(30-42)
🔇 Additional comments (5)
lib/fbe/octo.rb (1)
668-670: LGTM: deterministic extra issue for 'foo/bazz'Prepending 144 for this repo is clear and keeps ordering explicit.
test/fbe/test_octo.rb (4)
343-343: LGTM: covers 810→'foo/bazz' mappingAssertion makes the remap explicit.
422-435: LGTM: verifies fake issue shape for 'foo/bazz'Good structure checks on id/number/repo/user/pull_request.
437-441: LGTM: interface presence checksConfirms wrapper responds to auto‑pagination accessors.
467-484: LGTM: restoration after exception is exercisedGood coverage of ensure semantics.
|
@yegor256 can you take a look? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
lib/fbe/octo.rb (6)
202-211: Doc typo and YARD tags: fix example + param annotation
- Example should call list_issues (not list_issue).
- Use a named @yieldparam.
- Minor wording polish.
- # Disable auto pagination for octokit client called in block + # Temporarily disables auto pagination for the duration of the block. # # @yield [octo] Give octokit client with disabled auto pagination - # @yieldparam [Octokit::Client, Fbe::FakeOctokit] Octokit client + # @yieldparam octo [Octokit::Client, Fbe::FakeOctokit] Octokit client # @return [Object] Last value in block # @example # issue = # Fbe.octo.with_disable_auto_paginate do |octo| - # octo.list_issue('zerocracy/fbe', per_page: 1).first + # octo.list_issues('zerocracy/fbe', per_page: 1).first # end
212-218: Optional: enforce block presenceTo avoid a silent no-op when called without a block, consider raising.
def with_disable_auto_paginate - ap = @origin.auto_paginate + raise ArgumentError, 'block required' unless block_given? + ap = @origin.auto_paginate @origin.auto_paginate = false - yield self if block_given? + yield self ensure @origin.auto_paginate = ap end
272-275: FakeOctokit auto_paginate is a no-op; persist state for fidelityMimic Octokit by storing/restoring a boolean; improves tests that inspect the flag before/after.
+ def initialize + @auto_paginate = true + end - def auto_paginate=(_); end - def auto_paginate; end + def auto_paginate=(val); @auto_paginate = !!val; end + def auto_paginate; @auto_paginate; end
613-614: ID→full_name mapping leaves inconsistent owner/url fieldsFor Integer IDs (e.g., 810→'foo/bazz'), owner/login, html_url and url still use name (810) instead of full_name. Use full_name for those fields.
- owner: { login: name.to_s.split('/')[0], id: 526_301, site_admin: false }, - html_url: "https://github.com/#{name}", - url: "https://github.com/#{name}", + owner: { login: full_name.split('/').first, id: 526_301, site_admin: false }, + html_url: "https://github.com/#{full_name}", + url: "https://github.com/#{full_name}",Also, the summary claims the 1439→'zerocracy/baza' mapping was removed, but it’s still present here. If intentional, fine; otherwise, remove it.
677-680: Make Fake list_issues honor per_page/page to match OctokitImproves realism and validates with_disable_auto_paginate scenarios.
- ].tap do |list| - list.prepend(issue(repo, 144)) if repo == 'foo/bazz' - end + ].tap { |list| list.prepend(issue(repo, 144)) if repo == 'foo/bazz' } + per = _options[:per_page] + page = _options[:page] || 1 + return list unless per + list.slice((page - 1) * per, per) || []
717-725: Issue 144 lacks a state fieldAdd state (e.g., 'open') for parity with other issue shapes.
{ id: 657, number:, repo: { full_name: repo }, user: { login: 'yegor256', id: 526_301, type: 'User' }, pull_request: { merged_at: nil }, + state: 'open', created_at: Time.parse('2025-05-29 17:00:55 UTC') }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/fbe/octo.rb(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/fbe/octo.rb (1)
lib/fbe/issue.rb (1)
issue(30-42)
🔇 Additional comments (1)
lib/fbe/octo.rb (1)
212-218: Correct block-yield & restoration; good fix.Yields the decorated self, restores in ensure, safe on exceptions and nests correctly.
|
@rultor merge |
|
@rultor release, tag is |
|
@Yegorov Great job on your contribution! 🎉 Your effort has earned you +23 points: +24 base, +7 for 105 hits-of-code, and -8 for no code review. Remember, our policy rewards code reviews, so aim for that next time to maximize your points. Keep up the good work! Your balance is now +205. Don't forget to check your Zerocracy balance as well. |
|
@yegor256 Great job on the review! You've earned +13 points: +18 base, -12 for self-review, +7 for 467 hits-of-code, -5 for only 1 comment. Your balance is now +736. Remember to check your Zerocracy balance too. Keep up the good work! 💻 |
4 participants
Need for zerocracy/judges-action#1034
Summary by CodeRabbit
New Features
Tests