Skip to content

Conversation

@Patel-Raj11
Copy link
Collaborator

High Level Overview of Change

Ran - poetry update
https://python-poetry.org/docs/basic-usage/#updating-dependencies-to-their-latest-versions

Context of Change

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates
  • Release

Did you update CHANGELOG.md?

  • Yes
  • No, this change does not impact library users

Test Plan

Tests in CI should pass

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 22, 2025

Walkthrough

Poetry CLI version is updated from 2.1.1 to 2.2.1 across all four GitHub Actions workflows. Additionally, a safety check is added to xrpl/models/utils.py to prevent errors when accessing __kwdefaults__ by verifying it is not None before membership testing.

Changes

Cohort / File(s) Summary
CI/CD Workflow Poetry Version Updates
.github/workflows/faucet_test.yml, .github/workflows/integration_test.yml, .github/workflows/publish_to_pypi.yml, .github/workflows/unit_test.yml
Poetry CLI version environment variable bumped from 2.1.1 to 2.2.1 across all workflows, affecting installation and caching behavior.
KW_ONLY Safety Check
xrpl/models/utils.py
Added None-check guard before accessing "kw_only" membership in dataclass.__kwdefaults__ to prevent errors when the attribute is None across Python versions.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

Suggested reviewers

  • achowdhry-ripple
  • pdp2121

Poem

🐰 A rabbit in CI/CD dreams,
Bumped Poetry to newer schemes!
Safety checks now guard the way,
Stronger code to seize the day—
All versions dance in harmony! 🎭✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Title Check ⚠️ Warning The title “update dependencies” is overly generic and does not convey the primary changes in this pull request, which include bumping the Poetry version across multiple CI workflows and an internal safety fix in xrpl/models/utils.py. It fails to summarize the most important modifications, making it hard for reviewers to quickly grasp the content of the PR. Please revise the title to clearly reflect the main changes, for example “Bump Poetry to 2.2.1 in CI workflows and add safe None-check for kw_only in utils.”
Description Check ⚠️ Warning The description only states that poetry update was run and leaves the “Context of Change” section empty, does not list any specific changes, omits selection of the type of change, and provides only a minimal test plan. This does not fulfill the required template sections for summary, context, change type, or detailed test instructions. Please fill out the template by summarizing the specific workflow and code updates, providing context for why these changes were made, selecting the appropriate type of change checkbox, and expanding the Test Plan with steps or commands to verify the updates locally.
✅ Passed checks (1 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch update-dependencies

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

poetry.lock Outdated
# This file is automatically @generated by Poetry 2.2.0 and should not be changed by hand.
# This file is automatically @generated by Poetry 2.1.1 and should not be changed by hand.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't downgrade the Poetry version

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (4)
.github/workflows/unit_test.yml (1)

10-10: Poetry bump looks good; ensure Poetry bin is on PATH even on cache hits.

You only append $HOME/.local/bin to $GITHUB_PATH when the cache misses. On a cache hit, PATH may not include Poetry, causing “poetry: command not found”. Add an unconditional step to export PATH (or move the echo into its own always-run step). Also consider including runner.os/python-version in the cache key to avoid future cross-OS collisions.

.github/workflows/faucet_test.yml (1)

9-9: OK to bump; mirror the PATH safeguard.

Same PATH caveat as other workflows: export $HOME/.local/bin even when cache hits to avoid “poetry” not found. Current matrix spans Python 3.8–3.14; please confirm setup-python resolves 3.14 on the runner you use.

.github/workflows/integration_test.yml (1)

4-4: Bump is fine; add unconditional PATH export for Poetry.

Install step appends PATH only on cache miss. Add a tiny, always-run step to echo $HOME/.local/bin to $GITHUB_PATH. Everything else (timeout, matrix, docker health checks) looks solid.

.github/workflows/publish_to_pypi.yml (1)

12-12: Good; align actions/cache to v4 and ensure PATH export.

  • Use actions/cache@v4 (others already on v4) for consistency and performance.
  • Keep the PATH export for Poetry unconditional (not only on cache miss).
  • Nice touch verifying installation and artifact checks.
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4a5e54c and 94d1924.

⛔ Files ignored due to path filters (1)
  • poetry.lock is excluded by !**/*.lock
📒 Files selected for processing (5)
  • .github/workflows/faucet_test.yml (1 hunks)
  • .github/workflows/integration_test.yml (1 hunks)
  • .github/workflows/publish_to_pypi.yml (1 hunks)
  • .github/workflows/unit_test.yml (1 hunks)
  • xrpl/models/utils.py (1 hunks)

Comment on lines +301 to +303
return (
dataclass.__kwdefaults__ is not None and "kw_only" in dataclass.__kwdefaults__
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Null‑guard is correct; fix docstring mismatch and a small typo.

  • Change improves robustness when dataclass.kwdefaults is None. LGTM.
  • Docstring above (Lines 286–301) claims “older than 3.10 ⇒ True”, but the function returns True when kw_only is available (i.e., Python ≥ 3.10). Please correct it; also “higer” → “higher” in the comment on Line 306.
  • Optional: consider renaming to has_dataclass_kw_only() for clarity.
🤖 Prompt for AI Agents
In xrpl/models/utils.py around lines 301 to 303, the null-guard logic is fine
but the docstring on lines 286–301 incorrectly states “older than 3.10 ⇒ True”
while the function actually returns True when kw_only exists (Python ≥ 3.10);
update that docstring to state that presence of kw_only indicates Python 3.10 or
newer and correct the typo "higer" to "higher" on line 306; optionally, rename
the function to has_dataclass_kw_only() and update references if you want
clearer intent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants