#473: Make total_releases_published use conditional after cursor#497
Conversation
edmoffo
left a comment
There was a problem hiding this comment.
Fix for #473 is correct: total_releases_published now omits after on the first page just like the fixed code-review path in #444. Logic is straightforward and CI is green.
The diff also downgrades crate-ci/typos from v1.47.1 to v1.47.0 in .github/workflows/typos.yml. Drop that hunk; it has nothing to do with #473.
kreinba
left a comment
There was a problem hiding this comment.
The interpolation fix matches the pattern at lines 293, 356, and 410, but the no-cursor case is not covered by a regression test the way test_pull_requests_with_reviews_omits_after_when_cursor_is_nil and test_pull_request_reviews_omits_after_when_cursor_is_nil cover their siblings. One inline note about the missing companion test.
| total = 0 | ||
| cursor = nil | ||
| loop do | ||
| after = "after: \"#{cursor}\", " unless cursor.nil? |
There was a problem hiding this comment.
Three sibling methods that received the same fix in #444 each carry a regression test that captures the generated GraphQL string and asserts refute_includes(captured, 'after: ""') (test_github_graph.rb lines 293 and 306). Adding the equivalent test for total_releases_published would lock in the no-cursor case so a future edit cannot silently reintroduce after: "".
|
Added regression test for nil cursor following the sibling pattern. @kreinba |
…sibling methods
total_releases_published always interpolated after: "#{cursor}"
in the GraphQL query, even when cursor was nil. This produced
after: "" on the first page, which is an invalid pagination
argument — the same bug fixed for pull_requests_with_reviews
and pull_request_reviews in PR zerocracy#444.
Applied the same pattern used by total_commits_pushed (line 410),
pull_requests_with_reviews (line 293), and pull_request_reviews
(line 356): conditionally build the after argument only when
cursor is not nil.
371f700 to
60e4de4
Compare
|
@edmoffo Great work on the review! 🎉 You've earned +9 points (+18 base, -5 for limited comments, -4 for 16 hits-of-code). Your total score is now +1516 - check your Zerocracy account for updates! |
kreinba
left a comment
There was a problem hiding this comment.
The regression test on test_github_graph.rb:319 exercises the nil-cursor path that the prior review asked for, and the unrelated typos downgrade is gone. Diff is minimal at 15 lines across two files. CI is green.
|
@kreinba Thanks for the review! You've earned +9 points for this contribution. According to our work policy, you received +18 points as the baseline bonus for authoring a code review, then -5 points were deducted for having very few comments (only 1, when 9+ are expected), and -4 points for insufficient code size (16 hits-of-code, when 24+ are preferred). Your running score is +1639; don't forget to check your Zerocracy account too! 🎯 |
|
@VasilevNStas Hey! Nice work on your contribution! 🎉 You scored +20 points this time: started with the base +24, but we had to dock -4 since you only hit 16 hits-of-code (our policy dings anything under 30). Next time, try to beef up those contributions a bit more - you'll get 0.075 points per hit-of-code up to 16 bonus points, so bigger changes mean better rewards! Your running total is looking solid at +1658 - keep the momentum going and don't forget to peek at your Zerocracy account too! |
Description
total_releases_publishedinlib/fbe/github_graph.rb:497always interpolatesafter: "#{cursor}"in the GraphQL query. Whencursorisnil(first page), this producesafter: ""— an invalid pagination cursor.This is the same bug that PR #444 fixed for
pull_requests_with_reviewsandpull_request_reviews, buttotal_releases_publishedwas missed.Fix
Applied the same conditional interpolation pattern used by three sibling methods (
total_commits_pushed,pull_requests_with_reviews,pull_request_reviews):Changed the query from:
to:
Verification
bundle exec rubocop— 0 offensesbundle exec rake test— 283 tests, 0 failuresCloses #473