Restore library API and add full type annotations#150
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 14 minutes and 45 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR introduces a comprehensive AMQP client library implementation, adding core modules for channel management, message publishing/consumption, queue/exchange operations, transactions, and convenience APIs. Configuration updates for type checking and linting are also made. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Channel
participant Message
participant AMQP
participant RabbitMQ
Client->>Message: create(body, properties)
Message->>Message: normalize & populate (opinionated)
Client->>AMQP: basic_publish(exchange, routing_key, body)
AMQP->>Message: create message frame
AMQP->>Channel: write_frames(Publish, ContentHeader, ContentBody)
Channel->>RabbitMQ: send frames
alt Publisher Confirms Enabled
RabbitMQ->>Channel: Basic.Ack/Nack
Channel->>AMQP: wait_for_confirmation()
AMQP->>Client: return True/False
else No Confirms
Client->>Client: continue
end
sequenceDiagram
participant Client
participant Queue
participant Channel
participant AMQP
participant RabbitMQ
Client->>Queue: consume(no_ack=False, prefetch=10)
Queue->>Channel: register_consumer()
Queue->>AMQP: basic_consume()
AMQP->>Channel: write_frame(Basic.Consume)
Channel->>RabbitMQ: send consume
RabbitMQ->>Channel: Basic.Deliver + Content
Channel->>Queue: yield message
loop Until Queue Empty
Queue->>Client: message
Client->>Message: ack()
Message->>AMQP: basic_ack(delivery_tag)
AMQP->>Channel: write_frame(Basic.Ack)
Channel->>RabbitMQ: send ack
end
Client->>Queue: stop_consuming()
Queue->>AMQP: basic_cancel()
AMQP->>Channel: write_frame(Basic.Cancel)
Channel->>RabbitMQ: send cancel
sequenceDiagram
participant Client
participant SimpleChannel
participant Connection
participant Channel
participant Queue/Exchange
Client->>SimpleChannel: __enter__()
SimpleChannel->>Connection: create connection
Connection->>Connection: connect to RabbitMQ
SimpleChannel->>Channel: create channel
Channel->>Channel: open channel
SimpleChannel->>Client: return channel
Client->>Queue/Exchange: declare/bind/publish/consume
Queue/Exchange->>Channel: RPC operations
Client->>SimpleChannel: __exit__()
SimpleChannel->>Channel: close()
SimpleChannel->>Connection: close()
SimpleChannel->>Client: return
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Restores all source modules and tests from cd478a2^ that were wiped when the modernization commit staged pre-existing deletions: Source modules restored: - rabbitpy/amqp.py — AMQP adapter class (unopinionated channel API) - rabbitpy/amqp_queue.py — Queue class (was amqp_queue.py; keeps name) - rabbitpy/base.py — AMQPClass, AMQPChannel, StatefulObject base classes - rabbitpy/channel.py — full Channel implementation - rabbitpy/exchange.py — Exchange and typed exchange subclasses - rabbitpy/heartbeat.py — heartbeat thread - rabbitpy/message.py — Message class - rabbitpy/simple.py — one-shot helper functions (consume, get, publish) - rabbitpy/tx.py — Tx transaction class - rabbitpy/utils.py — utilities (urlparse, is_string, parse_qs, unquote, etc.) Removes empty rabbitpy/queue.py placeholder (logic lives in amqp_queue.py). Compatibility fixes for Python 3.11+ and pamqp 4: - Circular imports broken via typing.TYPE_CHECKING guard in base.py - pamqp.specification → pamqp.commands (renamed in pamqp 3+) - message._coerce_properties uses amqp_type() instead of __annotations__ (pamqp 4 uses __slots__, not annotations, for Properties fields) - datetime.datetime.utcnow() → datetime.datetime.now(tz=datetime.UTC) - import mock → from unittest import mock - import unittest2 → import unittest - import urlparse (Py2) → from urllib import parse - unicode() → str() in Python 2 skip-decorated tests - utils.PYTHON3 = True constant restored for skipIf decorators - utils.is_string(), parse_qs(), unquote() restored (tested and used) Tests restored: base_tests, channel_test, helpers, integration_tests, test_amqp, test_exchange, test_message, test_queue, test_tx, utils_tests. mypy/basedpyright overrides added for legacy modules (not yet annotated to strict standards) and legacy test files (intentionally test wrong types). Co-Authored-By: Gavin M. Roy <gavinmroy@gmail.com>
gmr
force-pushed
the
restore/deleted-modules
branch
from
f6270cf to
1d9f30f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rabbitpy/url_parser.py (1)
110-114:⚠️ Potential issue | 🟡 MinorBug:
sslrefers to the imported module, making the warning unreachable.At line 110,
use_ssl = bool(parsed.scheme == AMQPS and ssl)always evaluates toTruewhen the scheme is AMQPS becausesslis the imported module (always truthy). The warning at lines 113-114 can never execute sincenot sslis alwaysFalse.This appears to be dead code. If the intent is to conditionally enable SSL based on a parameter, the function signature needs a boolean parameter. If SSL availability should be checked, the Python
sslmodule is part of the standard library and always available in Python 3.4+, making this check unnecessary. Either remove the dead code or add a proper SSL enablement mechanism.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rabbitpy/url_parser.py` around lines 110 - 114, The code incorrectly treats the imported ssl module as a boolean, making the LOGGER.warning unreachable; change the function to accept a boolean parameter (e.g., ssl_enabled or enable_ssl) and use that instead of the ssl module: update the use_ssl assignment to use_ssl = bool(parsed.scheme == AMQPS and ssl_enabled) and update the conditional that logs the warning to if parsed.scheme == AMQPS and not ssl_enabled: LOGGER.warning('SSL requested but not available, disabling'); ensure the function signature, callers, and any tests are updated to pass the new boolean (default True if you want current behavior preserved).
🟡 Minor comments (13)
tests/base_tests.py-18-20 (1)
18-20:⚠️ Potential issue | 🟡 MinorType signature mismatch: test passes
bytesto parameter declared asstr.Line 19 passes
b'Foo'(bytes) toAMQPClass.__init__, but the parameter atrabbitpy/base.py:66is typed asname: str. This will trigger type checker warnings even though Python executes it without error at runtime (the bytes value is simply assigned toself.name).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/base_tests.py` around lines 18 - 20, The test passes bytes to AMQPClass but the __init__ signature for AMQPClass (rabbitpy/base.py: AMQPClass.__init__(self, name: str, ...)) is typed as name: str causing type-checker warnings; update the type annotation for the name parameter on AMQPClass.__init__ (and any related attribute annotations on AMQPClass) to accept both str and bytes (e.g., Union[str, bytes] or str | bytes) so the test and runtime behaviour align.tests/channel_test.py-56-59 (1)
56-59:⚠️ Potential issue | 🟡 MinorUse the real server-capability key in the nack guard test.
Channel._supports_basic_nackreads'basic.nack'. With'basic_nack'
here, the test passes only because the real key is absent, so it does not
actually protect the lookup.🩹 Suggested fix
- self.channel._server_capabilities['basic_nack'] = False + self.channel._server_capabilities['basic.nack'] = False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/channel_test.py` around lines 56 - 59, The test test_invoking_basic_nack_raises is setting the wrong server-capability key and therefore doesn’t exercise the nack guard; update the capability key on self.channel._server_capabilities from 'basic_nack' to the real key 'basic.nack' so Channel._supports_basic_nack sees it, then assert that calling self.channel._multi_nack(100) raises exceptions.NotSupportedError as intended.tests/test_exchange.py-47-65 (1)
47-65:⚠️ Potential issue | 🟡 MinorAssert the coerced
sourcevalue in the object-path tests.These checks still pass if
bind()/unbind()forward the object itself
instead ofval.name, because they only verify the AMQP class. Capture the
command and assertsource == 'bar'so the.nameconversion is actually
covered.🧪 Tighten the assertions
val = mock.Mock() val.name = 'bar' obj.bind(val, 'b') - self.assertIsInstance(rpc.mock_calls[0][1][0], - specification.Exchange.Bind) + command = rpc.call_args.args[0] + self.assertIsInstance(command, specification.Exchange.Bind) + self.assertEqual(command.source, 'bar') + self.assertEqual(command.routing_key, 'b') @@ val = mock.Mock() val.name = 'bar' obj.unbind(val, 'b') - self.assertIsInstance(rpc.mock_calls[0][1][0], - specification.Exchange.Unbind) + command = rpc.call_args.args[0] + self.assertIsInstance(command, specification.Exchange.Unbind) + self.assertEqual(command.source, 'bar') + self.assertEqual(command.routing_key, 'b')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_exchange.py` around lines 47 - 65, Update the two tests (test_bind_sends_exchange_bind_obj and test_bind_sends_exchange_unbind_obj) to not only assert the RPC command class (specification.Exchange.Bind / Unbind) but also to inspect the actual command object returned in rpc.mock_calls[0][1][0] and assert its source attribute equals 'bar' (verifying that bind()/unbind() forwarded val.name rather than the whole object); keep using the same mocked Exchange._rpc call and the same mock val with name = 'bar'.tests/test_tx.py-49-63 (1)
49-63:⚠️ Potential issue | 🟡 MinorReturn instances instead of class references in mock return values.
Tx.select()checksisinstance(response, commands.Tx.SelectOk)to set_selectedtoTrue. The tests return class references (e.g.,specification.Tx.SelectOk) instead of instances (e.g.,specification.Tx.SelectOk()), so the isinstance check fails and_selectedremainsFalse. This causescommit()androllback()to skip the transaction logic and hit theNoActiveTransactionErrorcheck instead.Update mock return values to return instances:
- Line 50:
specification.Tx.CommitOk→specification.Tx.SelectOk()- Line 59:
specification.Tx.SelectOk→specification.Tx.SelectOk()- Line 67: Add
rpc.return_value = specification.Tx.SelectOk()beforeobj.select()- Line 75:
specification.Tx.SelectOk→specification.Tx.SelectOk()- Line 82: Add
rpc.return_value = specification.Tx.SelectOk()beforeobj.select()- Also change
specification.Tx.CommitOktospecification.Tx.CommitOk()andspecification.Tx.RollbackOktospecification.Tx.RollbackOk()where they appear as return values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_tx.py` around lines 49 - 63, The tests currently set mock return values to class objects (e.g., specification.Tx.SelectOk) which breaks isinstance checks in tx.Tx.select/commit/rollback; update every mocked rpc.return_value in the failing tests to return instances instead (e.g., use specification.Tx.SelectOk(), specification.Tx.CommitOk(), specification.Tx.RollbackOk()) and ensure before calling obj.select() you set rpc.return_value = specification.Tx.SelectOk() so _selected gets set True and subsequent commit()/rollback() exercise the transaction paths.tests/utils_tests.py-63-64 (1)
63-64:⚠️ Potential issue | 🟡 MinorTypo in test method name:
test_unqoute→test_unquoteThe method name has a spelling error that should be corrected for clarity and discoverability.
🔤 Proposed fix
- def test_unqoute(self): + def test_unquote(self):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/utils_tests.py` around lines 63 - 64, Rename the misnamed test method test_unqoute to test_unquote so the spelling matches the util being tested (utils.unquote); update the test method definition and any references to it in the test class to preserve behavior (assertEqual(utils.unquote(self.PATH), '//')) and ensure test discovery picks it up.rabbitpy/exchange.py-128-128 (1)
128-128:⚠️ Potential issue | 🟡 MinorBug: Incorrect type annotation for
argumentsparameter.The type is
bool | Nonebut should bedict | Noneto match the parent class_Exchange.__init__signature and actual usage.🐛 Proposed fix
- arguments: bool | None = None): + arguments: dict | None = None):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rabbitpy/exchange.py` at line 128, The constructor's type annotation for the arguments parameter is incorrect (currently "arguments: bool | None") and should match the parent _Exchange.__init__ signature and usage; update the annotation to "arguments: dict | None" in the Exchange.__init__ (or equivalent constructor) so the parameter type aligns with _Exchange.__init__ and downstream code that treats arguments as a mapping.rabbitpy/exchange.py-163-165 (1)
163-165:⚠️ Potential issue | 🟡 MinorCopy-paste error in docstring.
The docstring says "direct exchanges" but this is the
HeadersExchangeclass.📝 Proposed fix
class HeadersExchange(_Exchange): - """The HeadersExchange class is used for interacting with direct exchanges + """The HeadersExchange class is used for interacting with headers exchanges only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rabbitpy/exchange.py` around lines 163 - 165, The HeadersExchange class docstring is incorrect (mentions "direct exchanges"); update the docstring on class HeadersExchange (subclass of _Exchange) to correctly describe that it represents a headers exchange type used for routing based on message header attributes and matching rules (e.g., x-match), replacing the current "direct exchanges" text with a concise, accurate description of headers exchanges and their purpose.rabbitpy/channel.py-1-7 (1)
1-7:⚠️ Potential issue | 🟡 MinorDocumentation typo: Exchange mentioned twice.
Line 4 references
Exchangetwice but the second reference should beQueue:-such as :py:class:`Exchange <rabbitpy.Exchange>` or -:py:class:`Exchange <rabbitpy.Queue>`. +such as :py:class:`Exchange <rabbitpy.Exchange>` or +:py:class:`Queue <rabbitpy.Queue>`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rabbitpy/channel.py` around lines 1 - 7, The module docstring incorrectly references Exchange twice; update the second Sphinx cross-reference to point to Queue instead (use :py:class:`Queue <rabbitpy.Queue>`), so the docstring reads that the base channel is used by objects such as :py:class:`Exchange <rabbitpy.Exchange>` or :py:class:`Queue <rabbitpy.Queue>`, leaving the rest of the docstring unchanged.rabbitpy/channel.py-109-124 (1)
109-124:⚠️ Potential issue | 🟡 MinorRe-raising exception loses traceback context.
Using
raise exc_valdirectly loses the original traceback. Use bareraiseto preserve it.Proposed fix
def __exit__( self, exc_type: type[BaseException] | None, exc_val: BaseException | None, unused_exc_tb: types.TracebackType | None, ): """When leaving the context, examine why the context is leaving, if it's an exception or what. """ if exc_type and exc_val: LOGGER.debug('Exiting due to exception: %r', exc_val) self._set_state(self.CLOSED) - raise exc_val + raise if self.open: self.close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rabbitpy/channel.py` around lines 109 - 124, In __exit__ of channel.Channel, don't re-raise the caught exception with raise exc_val because that discards the original traceback; instead preserve the original traceback by using a bare raise when exc_type/exc_val are present (after logging with LOGGER.debug and calling self._set_state(self.CLOSED)), so update the exception handling in __exit__ (the block that currently calls LOGGER.debug, self._set_state(self.CLOSED), and raise exc_val) to use bare raise.rabbitpy/message.py-326-351 (1)
326-351:⚠️ Potential issue | 🟡 MinorProperty coercion logic has redundant condition.
Line 350 uses
if python_type == 'datetime':withoutelif, meaning this check runs even after previous branches execute. Should beelif:Proposed fix
elif python_type == 'dict' and not isinstance(value, dict): LOGGER.warning('Resetting invalid value for %s to None', key) self.properties[key] = {} - if python_type == 'datetime': + elif python_type == 'datetime': self.properties[key] = self._as_datetime(value)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rabbitpy/message.py` around lines 326 - 351, The _coerce_properties method performs type-specific coercion but the final check uses if python_type == 'datetime' which runs even after earlier branches; change that final conditional to elif python_type == 'datetime' so datetime coercion (via self._as_datetime) only executes when no prior branch handled the property; update references in _coerce_properties that touch self.properties, _AMQP_TYPE_MAP, LOGGER, utils.maybe_utf8_encode and self._as_datetime accordingly.rabbitpy/base.py-173-175 (1)
173-175:⚠️ Potential issue | 🟡 Minor
__int__can returnNonewhen_channel_idis unset.The return type is
int, butself._channel_idcan beNone(as declared on line 164). This would cause a runtime error whenint(channel)is called before the channel is opened.Proposed fix
def __int__(self) -> int: """Return the numeric channel ID""" + if self._channel_id is None: + raise ValueError('Channel ID not yet assigned') return self._channel_idOr update the return type:
- def __int__(self) -> int: + def __int__(self) -> int | None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rabbitpy/base.py` around lines 173 - 175, The __int__ method currently returns self._channel_id which can be None; change __int__ (in class where _channel_id is declared) to guard for None: if self._channel_id is None raise a clear error (e.g., TypeError or RuntimeError with message like "channel ID unset: channel not opened") otherwise return the int value; update the docstring to state that calling int() on an unopened channel raises that error. Reference: method __int__ and attribute _channel_id.rabbitpy/message.py-163-168 (1)
163-168:⚠️ Potential issue | 🟡 MinorIncorrect return type annotation for
json()method.The method is annotated to return
bytes, butjson.loads()returnsAny(typicallydict,list,str,int,float,bool, orNone). The docstring says "Deserialize the message body if it is JSON, returning the value."Proposed fix
- def json(self) -> bytes: + def json(self) -> typing.Any: """Deserialize the message body if it is JSON, returning the value."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rabbitpy/message.py` around lines 163 - 168, The json method is incorrectly annotated as returning bytes but actually returns Python objects from json.loads; update the return type annotation on Message.json to typing.Any (or a more specific Union of JSON types) and add the necessary import (from typing import Any) at the top of the module; ensure both the try and except branches still return the deserialized value and update the docstring/type hint to match the new return type.rabbitpy/channel.py-472-502 (1)
472-502:⚠️ Potential issue | 🟡 MinorType mismatch in
_reject_inbound_messagecall.
_wait_for_content_framesaccepts a union typeBasic.Deliver | Basic.Get | Basic.Return, but_reject_inbound_messageis typed to only acceptBasic.Deliver. Whenmethod_frameisBasic.GetorBasic.Return, calling_reject_inbound_message(method_frame)at line 501 would be a type error.Consider updating the signature or adding a type guard:
Proposed fix
def _reject_inbound_message( - self, method_frame: commands.Basic.Deliver + self, method_frame: commands.Basic.Deliver | commands.Basic.GetOk ) -> None:Or guard the call:
+ if isinstance(method_frame, commands.Basic.Deliver): self._reject_inbound_message(method_frame)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rabbitpy/channel.py` around lines 472 - 502, The call to _reject_inbound_message(method_frame) in _wait_for_content_frames is a type mismatch because method_frame can be Basic.Get or Basic.Return but _reject_inbound_message only accepts Basic.Deliver; fix by adding a type guard before calling it: check isinstance(method_frame, commands.Basic.Deliver) and only call _reject_inbound_message when true (handle Basic.Get/Basic.Return paths separately or no-op), or alternatively change the _reject_inbound_message signature to accept the union type (commands.Basic.Deliver | commands.Basic.Get | commands.Basic.Return) and update its implementation/annotations accordingly; update references to _reject_inbound_message to match the new guard or signature.
🧹 Nitpick comments (15)
tests/base_tests.py (1)
26-29: Dead code: Python 2 unicode test is always skipped.
utils.PYTHON3is alwaysTruein modern Python, making this test permanently skipped. Since the PR removes Python 2 compatibility, consider removing this test entirely.🧹 Remove obsolete Python 2 test
- `@helpers.unittest.skipIf`(utils.PYTHON3, 'No unicode in Python 3') - def test_name_py2_unicode(self): - obj = base.AMQPClass(self.channel, 'Foo') - self.assertIsInstance(obj.name, str) -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/base_tests.py` around lines 26 - 29, Remove the obsolete Python 2 specific test method test_name_py2_unicode in tests/base_tests.py because utils.PYTHON3 is always true and the project no longer supports Py2; delete the entire method (the `@helpers.unittest.skipIf` decorator and def test_name_py2_unicode body that constructs base.AMQPClass(self.channel, 'Foo') and asserts isinstance(obj.name, str)) so the test suite no longer contains dead Py2-only checks.tests/test_queue.py (1)
141-145: Dead code: Python 2 unicode tests will always be skipped.
utils.PYTHON3is alwaysTrue(rabbitpy 3.x requires Python 3), so these tests are never executed. Consider removing them to reduce maintenance burden.Also applies to: 170-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_queue.py` around lines 141 - 145, Remove the Python2-only unicode tests that are permanently skipped: delete the test methods test_dlx_py2_unicode (and the other Py2 unicode test block referenced around lines 170-175) in tests/test_queue.py since utils.PYTHON3 is always True; locate the test functions by name (test_dlx_py2_unicode and the similar Py2 unicode test) and remove their definitions and any skipIf decorators so the test suite no longer contains dead Python2-specific cases.rabbitpy/connection.py (1)
128-131: Consider simplifying theargsproperty.The pattern
typing.cast(url_parser.ConnectionArgs, dict(self._args))creates a defensive copy (good), but the cast is applying TypedDict typing to a plain dict. Sinceself._argsis alreadyConnectionArgs, you could return it directly if mutation safety isn't a concern, or document why the copy is needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rabbitpy/connection.py` around lines 128 - 131, The args property is doing an unnecessary typing.cast on a defensive copy; either return the stored ConnectionArgs directly or keep the defensive copy but remove the misleading cast. Update the args property (the `@property` def args and its use of self._args) so that if mutation safety isn’t required you return self._args as a url_parser.ConnectionArgs, otherwise return dict(self._args) (and optionally wrap that in url_parser.ConnectionArgs if you need an explicit TypedDict conversion) and drop the typing.cast call; add a short docstring note explaining why you chose to copy or not for clarity.tests/integration_tests.py (1)
488-494: PreferassertRaisesfor cleaner exception testing.The current pattern is verbose. Using
assertRaiseswould be more consistent with other tests in this file.♻️ Proposed refactor
def test_raises_on_empty_name(self): - try: - for _msg in rabbitpy.consume(os.environ['RABBITMQ_URL']): - break - raise AssertionError('Did not raise ValueError') - except ValueError: - assert True + with self.assertRaises(ValueError): + for _msg in rabbitpy.consume(os.environ['RABBITMQ_URL']): + break🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration_tests.py` around lines 488 - 494, The test test_raises_on_empty_name uses a try/except loop to expect a ValueError from rabbitpy.consume, which is verbose; refactor it to use the test framework's assertRaises (or self.assertRaises) to assert the ValueError directly. Replace the manual try/except and loop that consumes from rabbitpy.consume(...) with a concise assertRaises context manager that calls the code that should raise (invoking rabbitpy.consume(os.environ['RABBITMQ_URL']) or the specific function call that triggers the ValueError) so the test asserts ValueError cleanly and consistently with other tests.tests/test_message.py (2)
456-459: Dead code: Python 2 unicode decode handling.Since rabbitpy 3.x requires Python 3, the
try/except AttributeErrorfor.decode('utf-8')is unnecessary. In Python 3,'☢'is already astr.🧹 Proposed cleanup
- try: - BODY = '☢'.decode('utf-8') - except AttributeError: - BODY = '☢' + BODY = '☢'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_message.py` around lines 456 - 459, Remove the Python 2 compatibility dead code around BODY: eliminate the try/except that attempts '☢'.decode('utf-8') and directly assign BODY = '☢' in tests/test_message.py (replace the entire try/except block that references BODY and AttributeError with a single direct assignment).
193-193: Mutable class attributes used as test fixtures.Ruff flags these as
RUF012. While technically mutable, these dicts are used read-only as test data and are a common unittest pattern. Converting totyping.ClassVarannotations would silence the warning if desired, but this is acceptable as-is for test code.Also applies to: 331-333, 378-380
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_message.py` at line 193, Tests define mutable dicts as class attributes (e.g., PROPERTIES) which triggers RUF012; annotate these as typing.ClassVar to indicate they're read-only test fixtures: import ClassVar from typing and change declarations like PROPERTIES = {...} to PROPERTIES: ClassVar[dict] = {...} (apply the same ClassVar annotation to the other mutable class-attribute dicts in this file).tests/utils_tests.py (1)
10-11: Dead code: Unicode compatibility hack and test are never executed.
utils.PYTHON3is alwaysTrue(perrabbitpy/utils.pyline 14), so the test at lines 56-58 will always be skipped, and theunicode = stralias at line 11 is unused.Consider removing this dead code since rabbitpy 3.x only supports Python 3.
🧹 Proposed cleanup
-# 3 Unicode Compatibility hack -unicode = str -- `@unittest.skipIf`(sys.version_info[0] == 3, 'No unicode obj in 3') - def test_is_string_unicode(self): - self.assertTrue(utils.is_string(unicode('Foo'))) -Also applies to: 56-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/utils_tests.py` around lines 10 - 11, Remove the dead Python2-compatibility alias and its corresponding skipped test: delete the unused "unicode = str" variable and remove or replace the test that checks Python2 behavior (the test relying on utils.PYTHON3 being False) in tests/utils_tests.py, since utils.PYTHON3 is always True; ensure no other code references the "unicode" symbol and update tests to cover relevant Python3 behavior instead.rabbitpy/base.py (3)
105-106: Unnecessary.keys()call.
in dict.keys()is equivalent toin dictbut less efficient:Proposed fix
- if value not in self.STATES.keys(): + if value not in self.STATES:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rabbitpy/base.py` around lines 105 - 106, Replace the unnecessary .keys() call on the STATES dict in the conditional: in the check in the method that currently uses "if value not in self.STATES.keys():", change it to test membership directly against the dict ("if value not in self.STATES:") so the lookup uses dict membership semantics and avoids creating a keys view; keep the ValueError raise (raise ValueError(f'Invalid state value: {value!r}')) unchanged.
424-432: Re-queuing non-matching frames may cause busy-wait loops.When
_wait_on_framereceives a frame that doesn't matchframe_type, it re-queues it (line 432) and continues looping. If the queue only contains non-matching frames, this creates a busy loop until timeout or state change. The 0.1s timeout in_read_from_queueprovides some mitigation, but consider if this is the intended behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rabbitpy/base.py` around lines 424 - 432, _wait_on_frame is re-queuing non-matching frames (using _read_queue.put) and immediately looping, which can create a busy-wait; change the logic in _wait_on_frame so that when a frame fails _validate_frame_type (after calling _check_for_rpc_request) you do not tight-loop: either temporarily stash non-matching frames (e.g. a local list) and only re-enqueue them after a wait/condition or yield to the event loop (small sleep or await a condition) before putting them back, ensuring _read_from_queue, _check_for_rpc_request, _validate_frame_type, _read_queue and the _waiting flag are used consistently to avoid starving other consumers.
91-91: Mutable class attributeSTATESflagged by Ruff.While
STATESis a dict (mutable), it's used as a lookup table and not modified at runtime. The Ruff RUF012 warning is technically correct but this is a false positive for this use case. You could silence it with a type annotation:STATES: typing.ClassVar[dict[int, str]] = {0: 'Closed', 1: 'Closing', 2: 'Open', 3: 'Opening'}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rabbitpy/base.py` at line 91, The mutable class attribute STATES triggers Ruff RUF012; annotate it as a class variable to silence the false positive by adding a type hint like typing.ClassVar[dict[int, str]] for STATES and ensure typing is imported in the module; update the declaration of STATES (the dict mapping 0..3 to strings) to include that ClassVar annotation so linters recognize it as an intentional immutable lookup table.rabbitpy/simple.py (1)
53-57: Parameter typebool | Nonewith defaultFalseis inconsistent.
no_ack: bool | None = FalsesuggestsNoneis a valid value, but the default isFalse. IfNonehas no special meaning, the type should just bebool:Proposed fix
- no_ack: bool | None = False, + no_ack: bool = False,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rabbitpy/simple.py` around lines 53 - 57, The consume function's no_ack parameter is typed as bool | None but given a default of False, which is inconsistent; either make the type a plain bool (no_ack: bool = False) if None has no special meaning, or change the default to None (no_ack: bool | None = None) and update any internal logic that treats None specially. Update the function signature for consume and adjust any subsequent checks/branches that reference the no_ack parameter to match the chosen representation (remove nullable checks if switching to bool, or handle None explicitly if keeping nullable).rabbitpy/amqp_queue.py (3)
296-309: PyPy-specific early return creates inconsistent behavior.The PyPy guard at line 304-305 returns silently when
not self.consuming, but the subsequent check at line 306-307 raisesNotConsumingErrorfor the same condition on other platforms. This inconsistency could mask programming errors on PyPy.Consider documenting why PyPy requires different behavior, or unifying the approach:
Proposed documentation
def stop_consuming(self) -> None: """Stop consuming messages. This is usually invoked if you want to cancel your consumer from outside the context manager or generator. If you invoke this, there is a possibility that the generator method will return None instead of a :py:class:`rabbitpy.Message`. """ + # PyPy: avoid raising in finalizers during interpreter shutdown if utils.PYPY and not self.consuming: return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rabbitpy/amqp_queue.py` around lines 296 - 309, The PyPy-specific early return in stop_consuming (the utils.PYPY && not self.consuming branch) causes inconsistent behavior versus other platforms where NotConsumingError is raised; remove the special-case early return so that when not self.consuming the method consistently raises exceptions.NotConsumingError on all runtimes, or if PyPy truly needs different handling, replace the silent return with an explicit documented behavior (e.g., a comment and still raising or logging) and ensure channel.cancel_consumer(self) and self.consuming toggling remain unchanged for the normal path.
65-73: Mutable class attributearguments = {}can cause shared state bugs.Class-level mutable defaults like
arguments = {}are shared across all instances. While the__init__reassignsself.arguments = arguments or {}, the class attribute itself could cause confusion or bugs if accessed directly viaQueue.arguments.This matches the Ruff RUF012 hint. Consider using
Noneas the class-level default or removing the class attribute entirely since__init__always sets it.Proposed fix
- arguments = {} + arguments: dict = None # type: ignore[assignment] # Set in __init__Or simply remove the class attribute since
__init__always initializes it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rabbitpy/amqp_queue.py` around lines 65 - 73, The class-level mutable attribute "arguments = {}" can cause shared-state bugs; update the class definition to use a non-mutable default (e.g., set "arguments = None") or remove the class-level "arguments" attribute entirely since __init__ already assigns self.arguments = arguments or {}. Locate the attribute in the class (reference symbol "arguments") and modify it so that only __init__ (the __init__ method) creates the per-instance dict, eliminating the mutable class-level default.
116-127: Return type annotation mismatch:IterablevsGenerator.
__iter__returns the result ofconsume(), which is aGenerator[message.Message, None, None]. The return type should match:Proposed fix
- def __iter__(self) -> typing.Iterable[message.Message]: + def __iter__(self) -> typing.Generator[message.Message, None, None]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rabbitpy/amqp_queue.py` around lines 116 - 127, The __iter__ method currently annotates its return as typing.Iterable[message.Message] but returns self.consume(), which is a Generator[message.Message, None, None]; update the return type annotation of __iter__ to typing.Generator[message.Message, None, None] (or typing.Iterator[message.Message] if you prefer a broader type) so it matches the actual return from consume() and references the consume function and message.Message type in the signature.rabbitpy/channel.py (1)
528-529: Barereturnin function returningMessage | None.Line 529 uses
returnwithout a value. While Python treats this asreturn None, explicitreturn Noneimproves clarity for typed code.Proposed fix
if error: - return + return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rabbitpy/channel.py` around lines 528 - 529, Replace the bare "return" inside the conditional that checks "if error:" with an explicit "return None" so the function that has a declared return type of "Message | None" clearly returns None in the error path; locate the "if error: return" snippet in rabbitpy/channel.py (the branch that handles errors when producing/obtaining a Message) and make that change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rabbitpy/amqp.py`:
- Around line 171-173: The call to message.Message in amqp.publish passes five
positional args though Message.__init__ expects four (channel, body_value,
properties=None, opinionated=False); remove the extra trailing False so the
constructor call becomes message.Message(self.channel, body, properties or {},
False) or use an explicit keyword for clarity (e.g., opinionated=False); then
keep returning msg.publish(exchange, routing_key, mandatory, immediate) as
before.
- Around line 38-46: Update the return type of basic_consume to reflect that it
yields messages (change from -> None to -> typing.Generator[message.Message,
None, None]) and add the required import (import typing) at the top of the file;
locate the generator method basic_consume and adjust its type annotation
accordingly so callers and type checkers know it yields message.Message
instances.
- Around line 115-126: basic_get currently sends the Basic.Get via
self._rpc(...) but discards the returned RPC result so callers never receive
Basic.GetOk/Basic.GetEmpty; change basic_get (the method in amqp.py) to capture
and return the value from self._rpc(commands.Basic.Get(0, queue, no_ack)) so the
caller can inspect the result (similar to how Queue.get() in amqp_queue.py uses
channel.get_message() after writing the frame); ensure the returned object is
the RPC response from _rpc rather than None.
In `@rabbitpy/base.py`:
- Line 229: The type annotation on write_frame is invalid; update the signature
of write_frame to use a proper union type for base.Frame and heartbeat.Heartbeat
(e.g., Union[base.Frame, heartbeat.Heartbeat] or base.Frame |
heartbeat.Heartbeat) and ensure you import typing.Union if using Union or use
Python 3.10+ PEP 604 syntax; keep the parameter name write_frame and the
referenced types base.Frame and heartbeat.Heartbeat unchanged so callers and
type checks resolve correctly.
In `@rabbitpy/heartbeat.py`:
- Around line 43-68: The stop()/_start_timer()/_maybe_send() logic needs a
thread-safe lifecycle guard: add a boolean attribute (e.g. self._stopped) and
use the existing self._lock to serialize changes so start(), stop(), and the
_maybe_send() callback cannot race; in stop() acquire the lock, set
self._stopped = True, cancel and null out any timer (and avoid del without
cancel), and release the lock; in _start_timer() acquire the lock, return
immediately if self._stopped, cancel any existing self._timer before replacing
it, then create/start the new Timer and release the lock; in _maybe_send()
acquire the lock at the top, check self._stopped before doing work (and before
calling _start_timer()), perform the bytes_written check and update
self._last_written while still holding the lock, release the lock and only then
call _start_timer() (or call it while holding the lock if you ensure no
deadlock), ensuring no timers are recreated after stop() is called.
In `@rabbitpy/simple.py`:
- Around line 130-169: The create_queue function currently defaults queue_name
to '' and calls _validate_name(queue_name, 'queue'), which makes the
empty-string default invalid and prevents RabbitMQ from auto-generating a name;
change the parameter to accept None (queue_name: str | None = None), update the
call site so that _validate_name is only invoked when queue_name is not
None/empty, and pass the unchanged queue_name (None or string) into
amqp_queue.Queue(...) so RabbitMQ can auto-generate the name when queue_name is
omitted; references: create_queue, _validate_name, SimpleChannel,
amqp_queue.Queue(...).declare().
In `@rabbitpy/tx.py`:
- Around line 91-92: The commit() and rollback() methods currently set
self._selected = False even when the operation succeeds, which breaks the Tx
context semantics; update both methods (commit(), rollback() in class Tx) so
that on success (when response is commands.Tx.CommitOk or
commands.Tx.RollbackOk) you do not clear self._selected (leave it True or
explicitly set True) and only clear it on failure/error paths, ensuring __exit__
will still see an active transaction for subsequent operations in the same
with-block.
- Around line 46-58: The __exit__ method is re-raising the caught exception with
"raise exc_val", which rebuilds the traceback and can be overwritten by rollback
errors; instead, in the __exit__ function (method __exit__) remove the "raise
exc_val" and return False after performing the LOGGER.warning and conditional
self.rollback() so that the original exception and its traceback are propagated
unchanged; ensure behavior when no exception remains unchanged (i.e., return
None/False appropriately).
In `@tests/base_tests.py`:
- Around line 35-36: The test expects a ValueError when a non-string name is
passed to AMQPClass, so update AMQPClass.__init__ to validate the name
parameter: inside AMQPClass.__init__ (the constructor that currently validates
channel), add an isinstance(name, str) check and raise ValueError with a clear
message if it fails (so test_name_invalid calling base.AMQPClass(self.channel,
1) will raise); ensure you reference the same parameter name `name` and keep
existing channel validation intact.
---
Outside diff comments:
In `@rabbitpy/url_parser.py`:
- Around line 110-114: The code incorrectly treats the imported ssl module as a
boolean, making the LOGGER.warning unreachable; change the function to accept a
boolean parameter (e.g., ssl_enabled or enable_ssl) and use that instead of the
ssl module: update the use_ssl assignment to use_ssl = bool(parsed.scheme ==
AMQPS and ssl_enabled) and update the conditional that logs the warning to if
parsed.scheme == AMQPS and not ssl_enabled: LOGGER.warning('SSL requested but
not available, disabling'); ensure the function signature, callers, and any
tests are updated to pass the new boolean (default True if you want current
behavior preserved).
---
Minor comments:
In `@rabbitpy/base.py`:
- Around line 173-175: The __int__ method currently returns self._channel_id
which can be None; change __int__ (in class where _channel_id is declared) to
guard for None: if self._channel_id is None raise a clear error (e.g., TypeError
or RuntimeError with message like "channel ID unset: channel not opened")
otherwise return the int value; update the docstring to state that calling int()
on an unopened channel raises that error. Reference: method __int__ and
attribute _channel_id.
In `@rabbitpy/channel.py`:
- Around line 1-7: The module docstring incorrectly references Exchange twice;
update the second Sphinx cross-reference to point to Queue instead (use
:py:class:`Queue <rabbitpy.Queue>`), so the docstring reads that the base
channel is used by objects such as :py:class:`Exchange <rabbitpy.Exchange>` or
:py:class:`Queue <rabbitpy.Queue>`, leaving the rest of the docstring unchanged.
- Around line 109-124: In __exit__ of channel.Channel, don't re-raise the caught
exception with raise exc_val because that discards the original traceback;
instead preserve the original traceback by using a bare raise when
exc_type/exc_val are present (after logging with LOGGER.debug and calling
self._set_state(self.CLOSED)), so update the exception handling in __exit__ (the
block that currently calls LOGGER.debug, self._set_state(self.CLOSED), and raise
exc_val) to use bare raise.
- Around line 472-502: The call to _reject_inbound_message(method_frame) in
_wait_for_content_frames is a type mismatch because method_frame can be
Basic.Get or Basic.Return but _reject_inbound_message only accepts
Basic.Deliver; fix by adding a type guard before calling it: check
isinstance(method_frame, commands.Basic.Deliver) and only call
_reject_inbound_message when true (handle Basic.Get/Basic.Return paths
separately or no-op), or alternatively change the _reject_inbound_message
signature to accept the union type (commands.Basic.Deliver | commands.Basic.Get
| commands.Basic.Return) and update its implementation/annotations accordingly;
update references to _reject_inbound_message to match the new guard or
signature.
In `@rabbitpy/exchange.py`:
- Line 128: The constructor's type annotation for the arguments parameter is
incorrect (currently "arguments: bool | None") and should match the parent
_Exchange.__init__ signature and usage; update the annotation to "arguments:
dict | None" in the Exchange.__init__ (or equivalent constructor) so the
parameter type aligns with _Exchange.__init__ and downstream code that treats
arguments as a mapping.
- Around line 163-165: The HeadersExchange class docstring is incorrect
(mentions "direct exchanges"); update the docstring on class HeadersExchange
(subclass of _Exchange) to correctly describe that it represents a headers
exchange type used for routing based on message header attributes and matching
rules (e.g., x-match), replacing the current "direct exchanges" text with a
concise, accurate description of headers exchanges and their purpose.
In `@rabbitpy/message.py`:
- Around line 326-351: The _coerce_properties method performs type-specific
coercion but the final check uses if python_type == 'datetime' which runs even
after earlier branches; change that final conditional to elif python_type ==
'datetime' so datetime coercion (via self._as_datetime) only executes when no
prior branch handled the property; update references in _coerce_properties that
touch self.properties, _AMQP_TYPE_MAP, LOGGER, utils.maybe_utf8_encode and
self._as_datetime accordingly.
- Around line 163-168: The json method is incorrectly annotated as returning
bytes but actually returns Python objects from json.loads; update the return
type annotation on Message.json to typing.Any (or a more specific Union of JSON
types) and add the necessary import (from typing import Any) at the top of the
module; ensure both the try and except branches still return the deserialized
value and update the docstring/type hint to match the new return type.
In `@tests/base_tests.py`:
- Around line 18-20: The test passes bytes to AMQPClass but the __init__
signature for AMQPClass (rabbitpy/base.py: AMQPClass.__init__(self, name: str,
...)) is typed as name: str causing type-checker warnings; update the type
annotation for the name parameter on AMQPClass.__init__ (and any related
attribute annotations on AMQPClass) to accept both str and bytes (e.g.,
Union[str, bytes] or str | bytes) so the test and runtime behaviour align.
In `@tests/channel_test.py`:
- Around line 56-59: The test test_invoking_basic_nack_raises is setting the
wrong server-capability key and therefore doesn’t exercise the nack guard;
update the capability key on self.channel._server_capabilities from 'basic_nack'
to the real key 'basic.nack' so Channel._supports_basic_nack sees it, then
assert that calling self.channel._multi_nack(100) raises
exceptions.NotSupportedError as intended.
In `@tests/test_exchange.py`:
- Around line 47-65: Update the two tests (test_bind_sends_exchange_bind_obj and
test_bind_sends_exchange_unbind_obj) to not only assert the RPC command class
(specification.Exchange.Bind / Unbind) but also to inspect the actual command
object returned in rpc.mock_calls[0][1][0] and assert its source attribute
equals 'bar' (verifying that bind()/unbind() forwarded val.name rather than the
whole object); keep using the same mocked Exchange._rpc call and the same mock
val with name = 'bar'.
In `@tests/test_tx.py`:
- Around line 49-63: The tests currently set mock return values to class objects
(e.g., specification.Tx.SelectOk) which breaks isinstance checks in
tx.Tx.select/commit/rollback; update every mocked rpc.return_value in the
failing tests to return instances instead (e.g., use
specification.Tx.SelectOk(), specification.Tx.CommitOk(),
specification.Tx.RollbackOk()) and ensure before calling obj.select() you set
rpc.return_value = specification.Tx.SelectOk() so _selected gets set True and
subsequent commit()/rollback() exercise the transaction paths.
In `@tests/utils_tests.py`:
- Around line 63-64: Rename the misnamed test method test_unqoute to
test_unquote so the spelling matches the util being tested (utils.unquote);
update the test method definition and any references to it in the test class to
preserve behavior (assertEqual(utils.unquote(self.PATH), '//')) and ensure test
discovery picks it up.
---
Nitpick comments:
In `@rabbitpy/amqp_queue.py`:
- Around line 296-309: The PyPy-specific early return in stop_consuming (the
utils.PYPY && not self.consuming branch) causes inconsistent behavior versus
other platforms where NotConsumingError is raised; remove the special-case early
return so that when not self.consuming the method consistently raises
exceptions.NotConsumingError on all runtimes, or if PyPy truly needs different
handling, replace the silent return with an explicit documented behavior (e.g.,
a comment and still raising or logging) and ensure channel.cancel_consumer(self)
and self.consuming toggling remain unchanged for the normal path.
- Around line 65-73: The class-level mutable attribute "arguments = {}" can
cause shared-state bugs; update the class definition to use a non-mutable
default (e.g., set "arguments = None") or remove the class-level "arguments"
attribute entirely since __init__ already assigns self.arguments = arguments or
{}. Locate the attribute in the class (reference symbol "arguments") and modify
it so that only __init__ (the __init__ method) creates the per-instance dict,
eliminating the mutable class-level default.
- Around line 116-127: The __iter__ method currently annotates its return as
typing.Iterable[message.Message] but returns self.consume(), which is a
Generator[message.Message, None, None]; update the return type annotation of
__iter__ to typing.Generator[message.Message, None, None] (or
typing.Iterator[message.Message] if you prefer a broader type) so it matches the
actual return from consume() and references the consume function and
message.Message type in the signature.
In `@rabbitpy/base.py`:
- Around line 105-106: Replace the unnecessary .keys() call on the STATES dict
in the conditional: in the check in the method that currently uses "if value not
in self.STATES.keys():", change it to test membership directly against the dict
("if value not in self.STATES:") so the lookup uses dict membership semantics
and avoids creating a keys view; keep the ValueError raise (raise
ValueError(f'Invalid state value: {value!r}')) unchanged.
- Around line 424-432: _wait_on_frame is re-queuing non-matching frames (using
_read_queue.put) and immediately looping, which can create a busy-wait; change
the logic in _wait_on_frame so that when a frame fails _validate_frame_type
(after calling _check_for_rpc_request) you do not tight-loop: either temporarily
stash non-matching frames (e.g. a local list) and only re-enqueue them after a
wait/condition or yield to the event loop (small sleep or await a condition)
before putting them back, ensuring _read_from_queue, _check_for_rpc_request,
_validate_frame_type, _read_queue and the _waiting flag are used consistently to
avoid starving other consumers.
- Line 91: The mutable class attribute STATES triggers Ruff RUF012; annotate it
as a class variable to silence the false positive by adding a type hint like
typing.ClassVar[dict[int, str]] for STATES and ensure typing is imported in the
module; update the declaration of STATES (the dict mapping 0..3 to strings) to
include that ClassVar annotation so linters recognize it as an intentional
immutable lookup table.
In `@rabbitpy/channel.py`:
- Around line 528-529: Replace the bare "return" inside the conditional that
checks "if error:" with an explicit "return None" so the function that has a
declared return type of "Message | None" clearly returns None in the error path;
locate the "if error: return" snippet in rabbitpy/channel.py (the branch that
handles errors when producing/obtaining a Message) and make that change.
In `@rabbitpy/connection.py`:
- Around line 128-131: The args property is doing an unnecessary typing.cast on
a defensive copy; either return the stored ConnectionArgs directly or keep the
defensive copy but remove the misleading cast. Update the args property (the
`@property` def args and its use of self._args) so that if mutation safety isn’t
required you return self._args as a url_parser.ConnectionArgs, otherwise return
dict(self._args) (and optionally wrap that in url_parser.ConnectionArgs if you
need an explicit TypedDict conversion) and drop the typing.cast call; add a
short docstring note explaining why you chose to copy or not for clarity.
In `@rabbitpy/simple.py`:
- Around line 53-57: The consume function's no_ack parameter is typed as bool |
None but given a default of False, which is inconsistent; either make the type a
plain bool (no_ack: bool = False) if None has no special meaning, or change the
default to None (no_ack: bool | None = None) and update any internal logic that
treats None specially. Update the function signature for consume and adjust any
subsequent checks/branches that reference the no_ack parameter to match the
chosen representation (remove nullable checks if switching to bool, or handle
None explicitly if keeping nullable).
In `@tests/base_tests.py`:
- Around line 26-29: Remove the obsolete Python 2 specific test method
test_name_py2_unicode in tests/base_tests.py because utils.PYTHON3 is always
true and the project no longer supports Py2; delete the entire method (the
`@helpers.unittest.skipIf` decorator and def test_name_py2_unicode body that
constructs base.AMQPClass(self.channel, 'Foo') and asserts isinstance(obj.name,
str)) so the test suite no longer contains dead Py2-only checks.
In `@tests/integration_tests.py`:
- Around line 488-494: The test test_raises_on_empty_name uses a try/except loop
to expect a ValueError from rabbitpy.consume, which is verbose; refactor it to
use the test framework's assertRaises (or self.assertRaises) to assert the
ValueError directly. Replace the manual try/except and loop that consumes from
rabbitpy.consume(...) with a concise assertRaises context manager that calls the
code that should raise (invoking rabbitpy.consume(os.environ['RABBITMQ_URL']) or
the specific function call that triggers the ValueError) so the test asserts
ValueError cleanly and consistently with other tests.
In `@tests/test_message.py`:
- Around line 456-459: Remove the Python 2 compatibility dead code around BODY:
eliminate the try/except that attempts '☢'.decode('utf-8') and directly assign
BODY = '☢' in tests/test_message.py (replace the entire try/except block that
references BODY and AttributeError with a single direct assignment).
- Line 193: Tests define mutable dicts as class attributes (e.g., PROPERTIES)
which triggers RUF012; annotate these as typing.ClassVar to indicate they're
read-only test fixtures: import ClassVar from typing and change declarations
like PROPERTIES = {...} to PROPERTIES: ClassVar[dict] = {...} (apply the same
ClassVar annotation to the other mutable class-attribute dicts in this file).
In `@tests/test_queue.py`:
- Around line 141-145: Remove the Python2-only unicode tests that are
permanently skipped: delete the test methods test_dlx_py2_unicode (and the other
Py2 unicode test block referenced around lines 170-175) in tests/test_queue.py
since utils.PYTHON3 is always True; locate the test functions by name
(test_dlx_py2_unicode and the similar Py2 unicode test) and remove their
definitions and any skipIf decorators so the test suite no longer contains dead
Python2-specific cases.
In `@tests/utils_tests.py`:
- Around line 10-11: Remove the dead Python2-compatibility alias and its
corresponding skipped test: delete the unused "unicode = str" variable and
remove or replace the test that checks Python2 behavior (the test relying on
utils.PYTHON3 being False) in tests/utils_tests.py, since utils.PYTHON3 is
always True; ensure no other code references the "unicode" symbol and update
tests to cover relevant Python3 behavior instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0089d636-ebb2-4752-8a0e-cd458ffb9dbf
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
.pre-commit-config.yamlpyproject.tomlrabbitpy/__init__.pyrabbitpy/amqp.pyrabbitpy/amqp_queue.pyrabbitpy/base.pyrabbitpy/channel.pyrabbitpy/channel0.pyrabbitpy/connection.pyrabbitpy/events.pyrabbitpy/exceptions.pyrabbitpy/exchange.pyrabbitpy/heartbeat.pyrabbitpy/io.pyrabbitpy/message.pyrabbitpy/py.typedrabbitpy/simple.pyrabbitpy/state.pyrabbitpy/tx.pyrabbitpy/url_parser.pyrabbitpy/utils.pytests/base_tests.pytests/channel_test.pytests/helpers.pytests/integration_tests.pytests/test_amqp.pytests/test_exchange.pytests/test_io.pytests/test_message.pytests/test_queue.pytests/test_tx.pytests/utils_tests.py
… by CI and CodeRabbit - Apply ruff format and fix 3 auto-fixable lint errors (unused import, unsorted imports) across helpers.py, integration_tests.py, utils_tests.py - Fix invalid type annotation syntax in base.AMQPChannel.write_frame: replace `[base.Frame, heartbeat.Heartbeat]` with proper union syntax - Add missing name type validation in base.AMQPClass.__init__ so test_name_invalid passes (raises ValueError for non-string names) - Fix basic_consume return type annotation to Generator[message.Message, None, None] (it is a generator) - Fix basic_get to return the RPC result instead of discarding it - Remove extra False argument in basic_publish Message constructor call - Add thread-safe _stopped flag to Heartbeat to prevent timer recreation after stop() is called; use lock to guard start/stop/_maybe_send lifecycle - Fix tx.Tx.__exit__ to return False instead of re-raising exc_val, preserving original exception traceback; wrap rollback in try/except - Fix tx.commit/rollback to keep _selected=True on success rather than always clearing it, matching the documented transaction semantics - Fix create_queue in simple.py to skip name validation when queue_name is empty, restoring RabbitMQ auto-naming behaviour Co-Authored-By: Gavin M. Roy <gavinmroy@gmail.com>
Owner
Author
|
🤖 This comment was posted by Claude on behalf of @gmr PR Monitor SummaryAll CI failures and CodeRabbit feedback have been addressed in commit CI FixesThe tests were failing on all Python versions (3.11-3.14) due to
CodeRabbit Feedback AddressedAll 9 threads resolved:
Test ResultsAll 199 unit tests pass locally ( |
TYPE_CHECKING-only imports (chan, conn) were used in class-body annotations evaluated eagerly on Python <3.14. Adding from __future__ import annotations defers annotation evaluation to strings on all supported versions, matching the behavior Python 3.14 provides by default. Co-Authored-By: Gavin M. Roy <gavinmroy@gmail.com>
exchange.py, amqp_queue.py, message.py, and tx.py all imported rabbitpy.channel at module level, but channel.py imports those same modules, creating a cycle. On Python 3.11-3.13 this caused an AttributeError at class definition time because the partially-initialized channel module had no Channel attribute yet. Fix by adding `from __future__ import annotations` (defers all annotation evaluation to runtime) and moving the channel import under `if typing.TYPE_CHECKING:`, matching the pattern already used in base.py. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
channel.py imported amqp and amqp_queue at module level for use in type annotations only, creating a circular import cycle: __init__ -> amqp -> channel -> amqp (circular). Python 3.14 evaluates annotations lazily by default, so the cycle was harmless there, but 3.11/3.12/3.13 evaluate class-body annotations at import time, causing AttributeError on the partially-initialised module. Fix follows the same pattern applied to exchange.py, amqp_queue.py, message.py, and tx.py: add `from __future__ import annotations` to defer all annotation evaluation, and move the amqp/amqp_queue imports under TYPE_CHECKING so they are invisible at runtime. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
The three MockServerConnectedTestCase tests checked _buffer and bytes_received immediately after write_protocol_header() (which only sleeps 100 ms) — before the IO thread had necessarily finished processing the server's response. On Python 3.12 CI runners, asyncio event-loop scheduling latency routinely exceeds 100 ms (300-400 ms observed in logs), so the assertions fired before the IO thread had received or dispatched any data. Fix: move all internal-state assertions (_buffer, bytes_received) to after the blocking queue.get() calls that are already present in each test. Those calls serve as natural synchronisation barriers: they only return once the IO thread has unmarshalled and enqueued the frame, guaranteeing that _buffer and bytes_received are in their final state. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
On Python 3.11 the asyncio writer callback registered on the IO thread's event loop fires more eagerly than on later versions. _on_write_ready is called both directly by the test and concurrently by the writer callback, enqueuing a second ConnectionException into self.exceptions. tearDown treats any remaining exception as an uncaught error and re-raises it. Fix by exiting the mock.patch context before yielding, then draining any duplicate ConnectionExceptions that the IO thread may have added before the write buffer was emptied. Each drained exception is asserted to be a ConnectionException so the drain itself is a test rather than silent suppression. Co-Authored-By: Gavin M. Roy <gavinmroy@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (12)
tests/integration_tests.py (1)
207-207: Minor inconsistency:message_bodyisstrhere butbytesin other tests.This test uses
self.message_body = 'ABC1234567890'(str) while other similar tests useb'...'(bytes). If intentional to test string handling, consider adding a comment; otherwise, align with other tests for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration_tests.py` at line 207, The test sets self.message_body as a str ('ABC1234567890') while other tests use bytes; update this for consistency by changing self.message_body to bytes (e.g., b'ABC1234567890') or, if the test intentionally verifies string handling, add a brief inline comment next to the self.message_body assignment explaining that this test covers str input so reviewers won't confuse it with the other byte-based tests; locate the assignment to self.message_body in the test class/method to make the change.rabbitpy/message.py (2)
380-381: Useelifinstead ofiffor mutually exclusive type checks.After handling the
dictcase, thedatetimecheck on line 380 usesifinstead ofelif. This causes the datetime coercion to run for all properties regardless of whether they were already handled above.♻️ Proposed fix
elif python_type == 'dict' and not isinstance(value, dict): LOGGER.warning('Resetting invalid value for %s to None', key) self.properties[key] = {} - if python_type == 'datetime': + elif python_type == 'datetime': self.properties[key] = self._as_datetime(value)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rabbitpy/message.py` around lines 380 - 381, Change the mutually exclusive type-check into a chain by replacing the standalone "if python_type == 'datetime'" with "elif python_type == 'datetime'" so that datetime coercion (self.properties[key] = self._as_datetime(value)) only runs when prior branches (e.g., the dict handling) did not already handle the property; locate the block using python_type, self.properties, and _as_datetime to apply the change.
366-370: Warning logged even when no coercion occurs.The warning at line 368 fires when the value is not already
bytesorstr, but then the coercion happens. However, when the value is alreadybytesorstr, no warning is needed—yet line 370 still encodes it. The warning message at line 368 should only log when actual coercion from a non-string type happens.♻️ Suggested fix
if python_type == 'str': if not isinstance(value, (bytes, str)): LOGGER.warning('Coercing property %s to bytes', key) value = str(value) - self.properties[key] = utils.maybe_utf8_encode(value) + self.properties[key] = utils.maybe_utf8_encode(value) + elif isinstance(value, str): + self.properties[key] = utils.maybe_utf8_encode(value)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rabbitpy/message.py` around lines 366 - 370, The current logic in rabbitpy/message.py logs a coercion warning unconditionally for the 'str' python_type branch; change the flow in the block handling python_type == 'str' so that LOGGER.warning('Coercing property %s to bytes', key) is only called when the incoming value is not an instance of (bytes, str) and you perform the coercion (i.e., inside the branch that converts non-string types to str), then always set self.properties[key] = utils.maybe_utf8_encode(value) after that; refer to the python_type variable, value local, LOGGER, utils.maybe_utf8_encode, and self.properties[key] to locate and update the code.tests/test_message.py (1)
467-470: Python 2 compatibility code is unnecessary.Since rabbitpy 3.x requires Python 3 (as noted in
utils.py), thetry/except AttributeErrorfor'☢'.decode('utf-8')is dead code. In Python 3, strings don't have adecodemethod, so theexceptbranch always executes.🧹 Proposed simplification
class TestPublishingUnicode(helpers.TestCase): - try: - BODY = '☢'.decode('utf-8') - except AttributeError: - BODY = '☢' + BODY = '☢' EXCHANGE = 'foo' ROUTING_KEY = 'bar.baz'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_message.py` around lines 467 - 470, Remove the Python‑2 compatibility branch around the BODY assignment: delete the try/except that attempts `'☢'.decode('utf-8')` and replace it with a single direct Unicode string assignment so BODY is set as BODY = '☢'; update any tests that referenced the decode behavior to use the plain BODY variable (symbol: BODY in tests/test_message.py).rabbitpy/utils.py (1)
91-108: Debug state is cached permanently and never refreshed.
_debuggingis set once on first access and never updated. If the log level changes at runtime (e.g., dynamic log level adjustment),_is_debuggingwill return stale information. Consider either documenting this limitation or refreshing the cache periodically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rabbitpy/utils.py` around lines 91 - 108, The _debugging cache in DebuggingOptimizationMixin is set once and never refreshed, so _is_debugging can return stale values if LOGGER's level changes; fix by either removing the cache and computing LOGGER.getEffectiveLevel() == logging.DEBUG each time in the _is_debugging property, or add an explicit invalidation mechanism (e.g., a clear_debug_cache() method or a setter that resets self._debugging) and call it when you change LOGGER's level; update the implementation around DebuggingOptimizationMixin, the _debugging attribute, and the _is_debugging property to ensure the cached value is refreshed or avoid caching entirely.tests/base_tests.py (1)
26-29: Dead test:PYTHON3is alwaysTrue.Since
rabbitpy/utils.pydefinesPYTHON3 = True(comment: "Always True — rabbitpy 3.x requires Python 3"), this test will always be skipped and can never execute. Consider removing this dead test code.🧹 Proposed removal
- `@helpers.unittest.skipIf`(utils.PYTHON3, 'No unicode in Python 3') - def test_name_py2_unicode(self): - obj = base.AMQPClass(self.channel, 'Foo') - self.assertIsInstance(obj.name, str) -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/base_tests.py` around lines 26 - 29, The test method test_name_py2_unicode is dead because utils.PYTHON3 is always True, so remove the entire test method (the `@helpers.unittest.skipIf`(...) decorator and def test_name_py2_unicode(...)) from tests/base_tests.py; locate the method referencing base.AMQPClass and utils.PYTHON3 and delete it to avoid a permanently skipped test, or alternatively replace it with a Python‑3 relevant assertion if behavior still needs coverage.rabbitpy/amqp_queue.py (4)
124-135: Return type could be more precise.
__iter__returnsself.consume()which is aGenerator, but the annotation saysIterable. WhileGeneratoris a subtype ofIterable, usingtyping.Generator[message.Message, None, None]would be more accurate and consistent withconsume()'s return type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rabbitpy/amqp_queue.py` around lines 124 - 135, The __iter__ method's return annotation is too generic: it currently declares typing.Iterable but actually returns self.consume(), which is a Generator; update the return type of __iter__ to typing.Generator[message.Message, None, None] to match consume() and be more precise (refer to the __iter__ method and consume() function and the message.Message type).
177-199: Type narrowing issue after reassignment.After
source = source.name, the variablesourceis now astr, but the type annotation still indicatesExchangeTypes. This may confuse type checkers. Consider using a separate variable for the string name:Suggested fix
def bind( self, source: exchange.ExchangeTypes, routing_key: str | None = None, arguments: dict | None = None, ) -> bool: - if hasattr(source, 'name'): - source = source.name + source_name = source.name if hasattr(source, 'name') else source frame = commands.Queue.Bind( queue=self.name, - exchange=source, + exchange=source_name, routing_key=routing_key or '', arguments=arguments, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rabbitpy/amqp_queue.py` around lines 177 - 199, The method bind reassigns source (annotated as exchange.ExchangeTypes) to source.name, narrowing it to str and confusing type checkers; fix by introducing a new variable (e.g., source_name) to hold source.name when hasattr(source, 'name') and use that variable in the commands.Queue.Bind call, leaving the original source parameter untouched; update references in the frame construction (queue=self.name, exchange=source_name or source) and keep the rest of bind (including _rpc and return check for commands.Queue.BindOk) unchanged.
63-63: Minor: Replace EN DASH with HYPHEN-MINUS in docstring.The docstring contains an ambiguous EN DASH (
–) character that should be replaced with a standard HYPHEN-MINUS (-) for consistency.- - **consumer_tag** (*str*) – Contains the consumer tag used to register + - **consumer_tag** (*str*) - Contains the consumer tag used to register🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rabbitpy/amqp_queue.py` at line 63, The docstring for the consumer_tag parameter uses an EN DASH character; open the docstring that documents "consumer_tag" in rabbitpy/amqp_queue.py (look for the line containing "**consumer_tag**") and replace the EN DASH (–) with a standard HYPHEN-MINUS (-) so the parameter description reads "**consumer_tag** (*str*) - Contains the consumer tag used to register" to ensure consistent ASCII punctuation across docs.
71-79: Consider usingClassVarfor mutable class-level defaults.The mutable default
arguments = {}at class level is flagged by static analysis (RUF012). While__init__always reassigns these as instance attributes, adding type annotations withClassVaror removing the class-level defaults would silence the warning and clarify intent.Suggested fix
+from typing import ClassVar + class Queue(base.AMQPClass): ... - arguments = {} + arguments: ClassVar[dict] = {}Or alternatively, remove the class-level defaults entirely since they're always overwritten in
__init__.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rabbitpy/amqp_queue.py` around lines 71 - 79, The class-level mutable default arguments = {} should be clarified to avoid RUF012: either annotate it as a typing.ClassVar (e.g., arguments: ClassVar[dict] = {}) to mark it as a class variable, or remove the class-level assignment entirely and only set self.arguments in __init__; update the class definition for arguments and keep __init__ assigning instance attributes (see arguments and __init__ in amqp_queue.py) so static analysis no longer flags the mutable default.rabbitpy/base.py (1)
106-106: Mutable class-level dict is acceptable here.The
STATESdict at line 106 is flagged by Ruff (RUF012), but this is a legitimate constant mapping that should be shared across instances. AddingClassVarannotation would clarify intent:+from typing import ClassVar + class StatefulObject(utils.DebuggingOptimizationMixin): - STATES = {0: 'Closed', 1: 'Closing', 2: 'Open', 3: 'Opening'} + STATES: ClassVar[dict[int, str]] = { + 0: 'Closed', 1: 'Closing', 2: 'Open', 3: 'Opening' + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rabbitpy/base.py` at line 106, The class-level mutable dict STATES should be declared as a constant ClassVar to silence RUF012 and convey intent; update the class to annotate STATES with typing.ClassVar (e.g., STATES: ClassVar[dict[int,str]] = {0: 'Closed', 1: 'Closing', 2: 'Open', 3: 'Opening'}) and add/import ClassVar from typing if not already imported so linters recognize it as a shared constant rather than an instance attribute.rabbitpy/simple.py (1)
42-53: Redundant exception re-raise in__exit__.The
if exc_type and exc_val: raiseblock is unnecessary. In__exit__, returningNone(orFalse) automatically propagates any exception. The bareraisehere is redundant and could be removed.Proposed fix
def __exit__( self, exc_type: type[BaseException] | None, exc_val: BaseException | None, unused_exc_tb: types.TracebackType | None, - ): + ) -> bool: if not self.channel.closed: self.channel.close() if not self.connection.closed: self.connection.close() - if exc_type and exc_val: - raise + return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rabbitpy/simple.py` around lines 42 - 53, The __exit__ method currently contains an unnecessary bare re-raise ("if exc_type and exc_val: raise") which is redundant because returning None/False will already propagate exceptions; remove that conditional re-raise from the __exit__ implementation in class/method __exit__ and keep the resource cleanup logic (close self.channel and self.connection only if not closed) so that exceptions propagate naturally without an explicit bare raise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rabbitpy/__init__.py`:
- Around line 53-70: The __all__ list includes module names (amqp_queue,
channel, connection, exceptions, exchange, message, simple, tx) that are not
actually imported at package level, causing AttributeError when accessing
rabbitpy.<module>; either import these submodules at the top-level (e.g., add
package-level imports like "from . import amqp_queue, channel, connection,
exceptions, exchange, message, simple, tx" so they become attributes) or remove
those names from the __all__ list so it only exposes actually imported symbols;
update the __all__ declaration accordingly and ensure consistency between
exported names and imported modules (check the existing imports around __all__
to mirror style).
In `@rabbitpy/amqp_queue.py`:
- Around line 312-314: The purge method sends a Queue.Purge frame without the
target queue name; update the call in purge() to include the queue identifier
(e.g. commands.Queue.Purge(queue=self.name) or
commands.Queue.Purge(queue=self._name) depending on the class attribute used for
the queue) so the server purges the correct queue; keep the rest of the method
intact and use the existing queue-name attribute used elsewhere in this class
(referencing purge() and commands.Queue.Purge()).
In `@rabbitpy/base.py`:
- Around line 443-492: The _wait_on_frame method can exit its loop without
returning a value, implicitly returning None which contradicts its annotated
return types; update the function signature (or union) to include
Optional[...]/None to reflect this and make the path explicit, then add a clear
terminal action: after the interrupt handling (after _on_interrupt_set() /
before method end) either raise a specific exception (e.g., RuntimeError or
custom) or add an explicit "return None" so callers see the intended outcome;
modify the return type annotation and adjust callers of _wait_on_frame
accordingly (references: _wait_on_frame, _on_interrupt_set, _read_from_queue,
_validate_frame_type).
- Around line 194-196: The __int__ method currently returns self._channel_id
which can be None (attribute _channel_id is typed int | None), violating the int
return annotation; update __int__ in class to explicitly guard for None and not
return it—if self._channel_id is None raise a TypeError (with a clear message
like "channel id is not set") or otherwise ensure an int is returned so the
function's signature remains correct; change only the body of __int__
(referencing __int__ and _channel_id) to perform this check and raise when
unset.
- Around line 235-250: The rpc method's annotated return type (base.Frame |
commands.Queue.DeclareOk) doesn't account for the implicit None returned when
frame_value.synchronous is False; update the signature of rpc to include None
(e.g., change the return annotation to base.Frame | commands.Queue.DeclareOk |
None or use Optional[...] / | None) or alternatively add an explicit "return
None" at the end of rpc so the implementation matches the declared type; edit
the rpc function in class Base (the rpc method that checks
frame_value.synchronous and calls _wait_on_frame) accordingly.
In `@rabbitpy/exchange.py`:
- Line 152: The type annotation for the arguments parameter is incorrect—change
the annotation on the Exchange constructor (or the function/method where
"arguments: bool | None = None" appears) to "dict | None" to match the parent
class and actual usage; update any typing imports if necessary so the signature
reads arguments: dict | None = None and ensure any docstrings or related type
hints/reference comments are consistent with dict | None.
- Around line 189-192: The HeadersExchange class docstring incorrectly says
"interacting with direct exchanges"; update the docstring of class
HeadersExchange to reference "headers exchanges" (or "headers-type exchanges")
so it accurately describes the exchange type handled by the HeadersExchange
class.
- Line 50: The class-level mutable default arguments = {} in rabbitpy.exchange
(the _Exchange / Exchange class) can produce shared-state bugs; remove the
mutable class attribute or mark it as a ClassVar and/or set it to None, and
ensure the instance attribute self.arguments is initialized in __init__ (leave
the existing self.arguments assignment intact) so no code relies on a shared
dict; if you want to keep a sentinel, use typing.ClassVar[dict] = {} or better
remove the class-level line entirely so only the instance attribute
(self.arguments) is used.
In `@rabbitpy/message.py`:
- Around line 250-253: The division calculating pieces can raise
ZeroDivisionError when self.channel.maximum_frame_size is 0; guard against that
by selecting a safe frame_size before the math.ceil call (e.g., frame_size =
self.channel.maximum_frame_size if self.channel.maximum_frame_size > 0 else
FALLBACK_FRAME_SIZE or 1) and then compute pieces = math.ceil(len(payload) /
float(frame_size)); update references to use this frame_size variable so payload
slicing logic uses a non-zero frame size.
- Around line 175-180: The json(self) method is annotated to return bytes but
actually returns the deserialized Python object (dict/list/primitives); change
the return type annotation from -> bytes to a proper typing type (e.g., -> Any
or a Union like dict[str, Any] | list[Any] | None) and add an import for
typing.Any if needed, and update the docstring to state it returns the
deserialized object; locate the json(self) method in message.py and adjust the
signature and docstring accordingly.
---
Nitpick comments:
In `@rabbitpy/amqp_queue.py`:
- Around line 124-135: The __iter__ method's return annotation is too generic:
it currently declares typing.Iterable but actually returns self.consume(), which
is a Generator; update the return type of __iter__ to
typing.Generator[message.Message, None, None] to match consume() and be more
precise (refer to the __iter__ method and consume() function and the
message.Message type).
- Around line 177-199: The method bind reassigns source (annotated as
exchange.ExchangeTypes) to source.name, narrowing it to str and confusing type
checkers; fix by introducing a new variable (e.g., source_name) to hold
source.name when hasattr(source, 'name') and use that variable in the
commands.Queue.Bind call, leaving the original source parameter untouched;
update references in the frame construction (queue=self.name,
exchange=source_name or source) and keep the rest of bind (including _rpc and
return check for commands.Queue.BindOk) unchanged.
- Line 63: The docstring for the consumer_tag parameter uses an EN DASH
character; open the docstring that documents "consumer_tag" in
rabbitpy/amqp_queue.py (look for the line containing "**consumer_tag**") and
replace the EN DASH (–) with a standard HYPHEN-MINUS (-) so the parameter
description reads "**consumer_tag** (*str*) - Contains the consumer tag used to
register" to ensure consistent ASCII punctuation across docs.
- Around line 71-79: The class-level mutable default arguments = {} should be
clarified to avoid RUF012: either annotate it as a typing.ClassVar (e.g.,
arguments: ClassVar[dict] = {}) to mark it as a class variable, or remove the
class-level assignment entirely and only set self.arguments in __init__; update
the class definition for arguments and keep __init__ assigning instance
attributes (see arguments and __init__ in amqp_queue.py) so static analysis no
longer flags the mutable default.
In `@rabbitpy/base.py`:
- Line 106: The class-level mutable dict STATES should be declared as a constant
ClassVar to silence RUF012 and convey intent; update the class to annotate
STATES with typing.ClassVar (e.g., STATES: ClassVar[dict[int,str]] = {0:
'Closed', 1: 'Closing', 2: 'Open', 3: 'Opening'}) and add/import ClassVar from
typing if not already imported so linters recognize it as a shared constant
rather than an instance attribute.
In `@rabbitpy/message.py`:
- Around line 380-381: Change the mutually exclusive type-check into a chain by
replacing the standalone "if python_type == 'datetime'" with "elif python_type
== 'datetime'" so that datetime coercion (self.properties[key] =
self._as_datetime(value)) only runs when prior branches (e.g., the dict
handling) did not already handle the property; locate the block using
python_type, self.properties, and _as_datetime to apply the change.
- Around line 366-370: The current logic in rabbitpy/message.py logs a coercion
warning unconditionally for the 'str' python_type branch; change the flow in the
block handling python_type == 'str' so that LOGGER.warning('Coercing property %s
to bytes', key) is only called when the incoming value is not an instance of
(bytes, str) and you perform the coercion (i.e., inside the branch that converts
non-string types to str), then always set self.properties[key] =
utils.maybe_utf8_encode(value) after that; refer to the python_type variable,
value local, LOGGER, utils.maybe_utf8_encode, and self.properties[key] to locate
and update the code.
In `@rabbitpy/simple.py`:
- Around line 42-53: The __exit__ method currently contains an unnecessary bare
re-raise ("if exc_type and exc_val: raise") which is redundant because returning
None/False will already propagate exceptions; remove that conditional re-raise
from the __exit__ implementation in class/method __exit__ and keep the resource
cleanup logic (close self.channel and self.connection only if not closed) so
that exceptions propagate naturally without an explicit bare raise.
In `@rabbitpy/utils.py`:
- Around line 91-108: The _debugging cache in DebuggingOptimizationMixin is set
once and never refreshed, so _is_debugging can return stale values if LOGGER's
level changes; fix by either removing the cache and computing
LOGGER.getEffectiveLevel() == logging.DEBUG each time in the _is_debugging
property, or add an explicit invalidation mechanism (e.g., a clear_debug_cache()
method or a setter that resets self._debugging) and call it when you change
LOGGER's level; update the implementation around DebuggingOptimizationMixin, the
_debugging attribute, and the _is_debugging property to ensure the cached value
is refreshed or avoid caching entirely.
In `@tests/base_tests.py`:
- Around line 26-29: The test method test_name_py2_unicode is dead because
utils.PYTHON3 is always True, so remove the entire test method (the
`@helpers.unittest.skipIf`(...) decorator and def test_name_py2_unicode(...)) from
tests/base_tests.py; locate the method referencing base.AMQPClass and
utils.PYTHON3 and delete it to avoid a permanently skipped test, or
alternatively replace it with a Python‑3 relevant assertion if behavior still
needs coverage.
In `@tests/integration_tests.py`:
- Line 207: The test sets self.message_body as a str ('ABC1234567890') while
other tests use bytes; update this for consistency by changing self.message_body
to bytes (e.g., b'ABC1234567890') or, if the test intentionally verifies string
handling, add a brief inline comment next to the self.message_body assignment
explaining that this test covers str input so reviewers won't confuse it with
the other byte-based tests; locate the assignment to self.message_body in the
test class/method to make the change.
In `@tests/test_message.py`:
- Around line 467-470: Remove the Python‑2 compatibility branch around the BODY
assignment: delete the try/except that attempts `'☢'.decode('utf-8')` and
replace it with a single direct Unicode string assignment so BODY is set as BODY
= '☢'; update any tests that referenced the decode behavior to use the plain
BODY variable (symbol: BODY in tests/test_message.py).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 94cb50e6-fba6-4dc9-aa1a-d8a2c086f107
📒 Files selected for processing (24)
pyproject.tomlrabbitpy/__init__.pyrabbitpy/amqp.pyrabbitpy/amqp_queue.pyrabbitpy/base.pyrabbitpy/channel.pyrabbitpy/exchange.pyrabbitpy/heartbeat.pyrabbitpy/message.pyrabbitpy/queue.pyrabbitpy/simple.pyrabbitpy/tx.pyrabbitpy/utils.pytests/base_tests.pytests/channel_test.pytests/helpers.pytests/integration_tests.pytests/test_amqp.pytests/test_exchange.pytests/test_io.pytests/test_message.pytests/test_queue.pytests/test_tx.pytests/utils_tests.py
✅ Files skipped from review due to trivial changes (4)
- tests/test_amqp.py
- tests/test_exchange.py
- tests/test_queue.py
- tests/test_tx.py
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/channel_test.py
- tests/helpers.py
- rabbitpy/heartbeat.py
- rabbitpy/tx.py
- tests/utils_tests.py
- pyproject.toml
- rabbitpy/channel.py
- __init__.py: remove unimported module names (amqp_queue, channel,
connection, exceptions, exchange, message, simple, tx) from __all__
to prevent AttributeError on access
- amqp_queue.py: pass queue=self.name to Queue.Purge() so the correct
queue is purged instead of the AMQP default empty-string queue
- base.py: guard __int__ against None _channel_id with ValueError;
update rpc() and _wait_on_frame() return types to include None to
match their implicit return paths
- exchange.py: remove mutable class-level arguments={} default that
would be shared across instances; fix Exchange.__init__ arguments
annotation from bool|None to dict|None; fix HeadersExchange docstring
typo ("direct" -> "headers")
- message.py: fix json() return type annotation from bytes to Any;
guard maximum_frame_size=0 in publish() with fallback to avoid
ZeroDivisionError
Co-Authored-By: Gavin M. Roy <gavinmroy@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two commits that together restore the complete rabbitpy public API (wiped in a prior modernization commit) and bring full static type checking to the library.
Problem
Commit
cd478a2inadvertently deleted the entire library implementation when it committed pre-staged deletions alongside the tooling modernization. The following modules and all associated tests were removed:amqp.py,amqp_queue.py,base.py,exchange.py,heartbeat.py,message.py,simple.py,tx.py,utils.py, andchannel.py(emptied). This left rabbitpy with no usable public API beyondConnection.Separately, the library had no
py.typedmarker, no type checker in CI or pre-commit, and partial/inconsistent type annotations.Solution
Restore library API (commit
f6270cf):cd478a2rabbitpy/queue.pyplaceholder — queue logic lives inamqp_queue.pytyping.TYPE_CHECKINGguard inbase.pypamqp.specification→pamqp.commands;Properties.__annotations__→amqp_type()(pamqp 4 uses__slots__)utcnow()→datetime.now(tz=datetime.UTC)unittest2,urlparse(Py2 module), bareimport mock,unicode()callsutils.is_string(),parse_qs(),unquote()helpersimport mock→from unittest import mockandpamqp.specification→pamqp.commandsthroughoutType annotations (commit
6a11518):rabbitpy/py.typed(PEP 561)mypy(strict) andbasedpyright(standard) to dev deps and pre-commit hooksSslOptionsandConnectionArgsTypedDicts inurl_parser.pyQueue/deque/defaultdictgenerics throughoutio.pyandconnection.pypamqp.frame.FrameTypesfrom pamqp 4 as the canonical frame union typeImpact
The full public API is restored:
AMQP,Channel,Connection,DirectExchange,Exchange,FanoutExchange,HeadersExchange,Message,Queue,SimpleChannel,TopicExchange,Tx,consume,create_*,delete_*,get,publish.199 unit tests pass. Pre-commit (ruff, mypy strict, basedpyright standard) all clean. Downstream consumers using mypy or pyright will now receive type information from rabbitpy.
Summary by CodeRabbit
Release Notes
New Features
publish(),consume(),get(), and queue/exchange management helpers.Tests
Chores