Find all converter subclasses / descendants #110
Conversation
|
Warning Rate limit exceeded@rlskoeser has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request updates the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Test/Caller
participant BDC as BaseDateConverter
participant Sub as Direct Subclass
Caller->>BDC: call subclasses()
BDC->>BDC: Retrieve __subclasses__()
loop For each direct subclass
BDC->>Sub: call subclasses()
Sub-->>BDC: Return nested subclasses
end
BDC->>BDC: Exclude BaseCalendarConverter
BDC-->>Caller: Return aggregated subclass set
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/undate/converters/base.py (1)
117-121: Clarify the docstring to reflect the returned set.The docstring mentions "List of available converters," but the method now returns a set. Updating the docstring for accuracy would help maintain clarity.
Here's a suggested fix:
- List of available converters classes. Includes calendar convert + Set of available converter classes. Includes calendar converter
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/undate/converters/base.py(2 hunks)tests/test_converters/test_base.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: python-unit (3.13)
- GitHub Check: python-unit (3.12)
- GitHub Check: python-unit (3.11)
- GitHub Check: python-unit (3.10)
- GitHub Check: python-unit (3.9)
🔇 Additional comments (4)
src/undate/converters/base.py (2)
126-132: Validate recursion safety.This recursive approach effectively gathers nested subclasses. Should a rare case of cyclic inheritance arise (which Python disallows by default), it could cause issues, but given standard class inheritance rules, this is safe and correct.
133-134: Excluding BaseCalendarConverter seems appropriate.Removing the base calendar converter from the final set aligns with the documented intent to exclude non-functional base implementations.
tests/test_converters/test_base.py (2)
5-9: Selective imports are clean and concise.Listing only the required calendar converters is a good practice that promotes clarity in tests.
37-48: Thorough subclass coverage test.Defining a nested subclass and verifying its inclusion ensures proper coverage of the recursive subclass logic. This test appropriately confirms that the base calendar converter is excluded while derived converters are included.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/undate/converters/base.py (1)
127-136: Implementation looks good but remove the debug print.The recursive implementation correctly finds all nested subclasses while excluding
BaseCalendarConverter. However, there's a commented out debug print statement that should be removed.- # print(f"class subclasses: {subc.name} {subc.subclasses()}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/undate/converters/base.py(1 hunks)
🔇 Additional comments (3)
src/undate/converters/base.py (3)
117-117: LGTM! Good choice using a set return type.The change from list to set is appropriate since order doesn't matter for converter subclasses and sets prevent duplicates while enabling cleaner set operations.
119-121: LGTM! Documentation accurately reflects the changes.The docstring clearly explains that descendant subclasses are included while
BaseCalendarConverteris excluded.
127-133: Verify that nested calendar converters are discovered correctly.Let's confirm that the recursive implementation correctly discovers nested calendar converters, particularly for the Hebrew calendar use case mentioned in the PR description.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check if nested calendar converters are discovered correctly # Search for Hebrew calendar converter and its subclasses ast-grep --pattern $'class HebrewDateConverter { $$$ }' # Search for any classes that extend HebrewDateConverter ast-grep --pattern $'class $_ extends HebrewDateConverter { $$$ }'Length of output: 128
I'll generate a new shell script to search for Hebrew calendar converters using regex patterns suitable for Python syntax. This will help us verify whether HebrewDateConverter and its subclasses are defined in the codebase.
🏁 Script executed:
#!/bin/bash echo "Searching for the declaration of HebrewDateConverter:" rg "class\s+HebrewDateConverter\s*\(" -n echo "" echo "Searching for any classes inheriting from HebrewDateConverter:" rg "class\s+[A-Za-z0-9_]+\s*\(.*HebrewDateConverter.*\)\s*:" -nLength of output: 476
Nested Converter Discovery Works as Intended
The recursive subclass collection in the base converter (using subclasses() and set unions) is correctly implemented. Our searches confirm that HebrewDateConverter is defined, and although no additional subclasses were found for it in the Hebrew calendar module, the recursive logic will discover any nested converters if they are added.
Revises converter subclass logic to find nested / descendant classes.
This resolves an issue I discovered when I tried to subclass the Hebrew converter to add a Seleucid calendar converter and the subclass wasn't being found.
Summary by CodeRabbit