-
Notifications
You must be signed in to change notification settings - Fork 27
Add more linting rules #642
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis pull request modernizes the Python codebase by systematically converting legacy string formatting to f-strings, updating type annotations from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
cf63830 to
33b9d3c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ably/realtime/connectionmanager.py (1)
123-147: Fix swappedAblyExceptionarguments inping()and consider concurrent-call behaviourInside
ping(), this branch:else: raise AblyException("Cannot send ping request. Calling ping in invalid state", 40000, 400)appears to have
status_codeandcodeswapped compared to other call sites in this file (which usestatus_codeas a 3‑digit HTTP code andcodeas a 5‑digit Ably error code). That likely wants to be400, 40000instead.Also note that when
self.__ping_futureis already set, the early path returns the raw future result (currentlyNonefromon_heartbeat) instead of afloat, so concurrentping()callers won’t receive a latency measurement as per the return type. Not critical for this PR, but worth tightening in a follow‑up (e.g. by either disallowing overlapping pings or sharing a latency-valued future).Suggested minimal fix for the argument ordering:
- else: - raise AblyException("Cannot send ping request. Calling ping in invalid state", 40000, 400) + else: + raise AblyException("Cannot send ping request. Calling ping in invalid state", 400, 40000)
🧹 Nitpick comments (10)
ably/types/device.py (1)
19-19: F-string conversion looks good!The modernization from old-style string formatting to f-strings is correctly implemented across all three validation error messages. The changes align well with the linting improvements in this PR.
Consider using the
!rformat specifier for better debugging output:- raise ValueError(f'unexpected transport type {transport_type}') + raise ValueError(f'unexpected transport type {transport_type!r}')- raise ValueError(f'unexpected platform {platform}') + raise ValueError(f'unexpected platform {platform!r}')- raise ValueError(f'unexpected form factor {form_factor}') + raise ValueError(f'unexpected form factor {form_factor!r}')Using
!rensures the error messages show therepr()of invalid values, making it easier to distinguish edge cases likeNone, empty strings, or unexpected types during debugging.Also applies to: 22-22, 25-25
test/ably/rest/restchannels_test.py (1)
63-71: F‑string change is good; consider replacing list comprehension with a loopThe move to
channel_names = [f'channel_{i}' for i in range(5)]is clear and equivalent to the previous formatting. However, the next line:[self.ably.channels.get(name) for name in channel_names]uses a list comprehension purely for side effects, which the new bugbear/comprehensions rules are likely to flag. Consider a simple loop instead:
- [self.ably.channels.get(name) for name in channel_names] + for name in channel_names: + self.ably.channels.get(name)This keeps the behavior while avoiding an unnecessary transient list and stays lint‑friendly.
ably/util/helper.py (1)
1-14: Import cleanup and minor comprehension tweak are fine; small style nitThe import reordering and more explicit imports are fine and keep this helper aligned with modern style. For
get_random_id, you can make the unused loop variable intention clearer:- random_id = ''.join(random.choice(source) for i in range(8)) + random_id = ''.join(random.choice(source) for _ in range(8))Purely cosmetic; current behavior is correct.
ably/rest/push.py (1)
4-8: Push admin imports and f-string updates look correct
- Explicitly importing
PushChannelSubscriptionand the response processors aligns with how they’re used later and keeps dependencies clear.- The new
TypeErrormessages inpublishpreserve the validation logic while giving more informative error text.- Using f-strings for the device registration URLs keeps the paths the same as before.
If there’s any chance
device_idordevice_details.idcan contain reserved URL characters, consider quoting the segment, but that’s outside the scope of this linting-focused change.Also applies to: 50-54, 82-83, 106-107, 118-119
ably/util/crypto.py (1)
119-119: Unnecessary parentheses around f-string.The parentheses around the f-string are not needed since there's no tuple or other expression requiring them.
- return (f"{self.__algorithm}-{self.__key_length}-{self.__mode}").lower() + return f"{self.__algorithm}-{self.__key_length}-{self.__mode}".lower()test/ably/rest/restcrypto_test.py (1)
84-84: Inconsistent string formatting style.This line uses
.format()while other log statements (lines 46-47, 78) use f-strings. For consistency, consider using f-strings throughout:- "Expect publish4 to be expected byte[]. Actual: {}".format(str(message_contents['publish4'])) + f"Expect publish4 to be expected byte[]. Actual: {message_contents['publish4']}"The same inconsistency appears on lines 117 and 166.
test/ably/rest/restauth_test.py (1)
92-92: Inconsistent formatting style with line 120.Line 92 uses
.format()for the Basic auth header while line 120 uses an f-string for the Bearer token. Consider using f-strings consistently:- assert authorization == 'Basic {}'.format(base64.b64encode('bar:foo'.encode('ascii')).decode('utf-8')) + assert authorization == f"Basic {base64.b64encode('bar:foo'.encode('ascii')).decode('utf-8')}"test/ably/rest/restchannelpublish_test.py (1)
66-66: Inconsistent formatting style.Uses
.format()while line 60 uses f-string. Consider using f-strings consistently for error messages.ably/rest/auth.py (1)
232-232: Minor inconsistency: Uses .format() while other logs use f-strings.For consistency with other log statements in this file (lines 169-170, 183):
- log.debug("Token: {}".format(str(response_dict.get("token")))) + log.debug(f"Token: {response_dict.get('token')}")ably/realtime/connectionmanager.py (1)
1-8: Union typing and__future__usage look good; a couple of minor typing/robustness tweaks to considerThe move to
from __future__ import annotationsandX | Noneunions across the connection manager, timers, and transport/task fields is consistent and improves readability.Two low-impact suggestions:
- In
connect_with_fallback_hosts, prefer a more precise parameter type, e.g.fallback_hosts: list[str], to matchself.__fallback_hosts: list[str].- Still in
connect_with_fallback_hosts,exceptioncan be referenced before assignment iffallback_hostsis ever empty; defensively initializing it (e.g.exception: Exception | None = Nonebefore the loop, and guarding the finalreturn) will keep static checkers happy and avoid surprises if new call sites pass an empty list.Also applies to: 32-45, 48-48, 149-149, 239-239, 247-247, 281-281, 349-350, 389-389, 526-526
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (58)
ably/__init__.py(1 hunks)ably/http/http.py(2 hunks)ably/http/httputils.py(1 hunks)ably/http/paginatedresult.py(2 hunks)ably/realtime/connection.py(4 hunks)ably/realtime/connectionmanager.py(9 hunks)ably/realtime/realtime.py(1 hunks)ably/realtime/realtime_channel.py(10 hunks)ably/rest/auth.py(10 hunks)ably/rest/channel.py(7 hunks)ably/rest/push.py(5 hunks)ably/rest/rest.py(2 hunks)ably/scripts/unasync.py(1 hunks)ably/transport/websockettransport.py(2 hunks)ably/types/authoptions.py(1 hunks)ably/types/capability.py(1 hunks)ably/types/channelstate.py(1 hunks)ably/types/connectionstate.py(1 hunks)ably/types/device.py(1 hunks)ably/types/message.py(2 hunks)ably/types/mixins.py(2 hunks)ably/types/options.py(2 hunks)ably/types/presence.py(2 hunks)ably/types/tokenrequest.py(1 hunks)ably/types/typedbuffer.py(3 hunks)ably/util/case.py(0 hunks)ably/util/crypto.py(3 hunks)ably/util/eventemitter.py(1 hunks)ably/util/exceptions.py(6 hunks)ably/util/helper.py(1 hunks)pyproject.toml(1 hunks)test/ably/conftest.py(1 hunks)test/ably/realtime/eventemitter_test.py(1 hunks)test/ably/realtime/realtimeauth_test.py(1 hunks)test/ably/realtime/realtimechannel_test.py(1 hunks)test/ably/realtime/realtimechannel_vcdiff_test.py(1 hunks)test/ably/realtime/realtimeconnection_test.py(1 hunks)test/ably/realtime/realtimeinit_test.py(1 hunks)test/ably/realtime/realtimeresume_test.py(1 hunks)test/ably/rest/encoders_test.py(1 hunks)test/ably/rest/restauth_test.py(8 hunks)test/ably/rest/restcapability_test.py(1 hunks)test/ably/rest/restchannelhistory_test.py(5 hunks)test/ably/rest/restchannelpublish_test.py(8 hunks)test/ably/rest/restchannels_test.py(1 hunks)test/ably/rest/restchannelstatus_test.py(1 hunks)test/ably/rest/restcrypto_test.py(6 hunks)test/ably/rest/resthttp_test.py(3 hunks)test/ably/rest/restinit_test.py(4 hunks)test/ably/rest/restpaginatedresult_test.py(1 hunks)test/ably/rest/restpresence_test.py(2 hunks)test/ably/rest/restpush_test.py(3 hunks)test/ably/rest/restrequest_test.py(2 hunks)test/ably/rest/reststats_test.py(1 hunks)test/ably/rest/resttime_test.py(2 hunks)test/ably/rest/resttoken_test.py(1 hunks)test/ably/testapp.py(2 hunks)test/ably/utils.py(2 hunks)
💤 Files with no reviewable changes (1)
- ably/util/case.py
🧰 Additional context used
🧬 Code graph analysis (31)
test/ably/realtime/realtimeconnection_test.py (2)
ably/types/connectionstate.py (2)
ConnectionEvent(19-28)ConnectionState(8-16)ably/transport/defaults.py (1)
Defaults(1-63)
test/ably/rest/restchannelhistory_test.py (3)
ably/rest/channel.py (4)
ably(145-146)publish(108-134)history(31-38)name(149-150)ably/http/paginatedresult.py (4)
PaginatedResult(44-111)items(56-57)next(71-72)first(68-69)ably/types/message.py (1)
name(65-66)
ably/types/authoptions.py (1)
ably/util/exceptions.py (1)
AblyException(9-84)
test/ably/rest/restpush_test.py (2)
ably/util/exceptions.py (2)
AblyAuthException(99-100)AblyException(9-84)test/ably/utils.py (5)
BaseAsyncTestCase(44-59)VaryByProtocolTestsMetaclass(120-157)get_random_key(175-176)new_dict(169-172)random_string(165-166)
test/ably/rest/resttoken_test.py (1)
test/ably/utils.py (1)
patch(77-87)
ably/rest/channel.py (1)
ably/util/exceptions.py (2)
IncompatibleClientIdException(103-104)catch_all(87-96)
test/ably/rest/restpresence_test.py (2)
test/ably/utils.py (3)
BaseAsyncTestCase(44-59)VaryByProtocolTestsMetaclass(120-157)dont_vary_protocol(160-162)ably/types/presence.py (3)
member_key(80-82)connection_id(64-65)client_id(60-61)
test/ably/rest/restcrypto_test.py (1)
ably/util/crypto.py (6)
CipherParams(16-42)generate_random_key(138-140)get_cipher(163-168)get_default_params(143-160)iv(33-34)iv(114-115)
ably/types/mixins.py (3)
ably/util/exceptions.py (1)
AblyException(9-84)ably/types/presence.py (1)
encoding(72-73)ably/util/crypto.py (1)
CipherData(122-131)
ably/rest/rest.py (2)
ably/http/paginatedresult.py (2)
HttpPaginatedResponse(114-134)PaginatedResult(44-111)ably/util/exceptions.py (2)
AblyException(9-84)catch_all(87-96)
ably/realtime/connection.py (4)
ably/util/exceptions.py (1)
AblyException(9-84)ably/realtime/realtime_channel.py (1)
error_reason(568-570)ably/realtime/connectionmanager.py (1)
connection_details(526-527)ably/types/connectiondetails.py (1)
ConnectionDetails(5-20)
ably/types/message.py (1)
ably/types/mixins.py (2)
DeltaExtras(13-19)EncodeDataMixin(29-130)
test/ably/realtime/realtimechannel_test.py (4)
ably/types/connectionstate.py (1)
ConnectionState(8-16)ably/realtime/realtime_channel.py (2)
ChannelOptions(26-87)RealtimeChannel(90-593)ably/types/channelstate.py (1)
ChannelState(8-15)ably/util/exceptions.py (1)
AblyException(9-84)
test/ably/rest/restchannelpublish_test.py (5)
ably/rest/push.py (3)
ably(31-32)ably(72-73)ably(137-138)ably/http/http.py (3)
auth(267-268)auth(271-272)url(67-68)ably/types/message.py (3)
Message(24-231)name(65-66)data(69-70)test/ably/utils.py (4)
BaseAsyncTestCase(44-59)VaryByProtocolTestsMetaclass(120-157)assert_waiter(187-199)dont_vary_protocol(160-162)ably/util/case.py (1)
snake_to_camel(12-17)
ably/http/http.py (2)
ably/http/httputils.py (1)
HttpUtils(8-55)ably/util/helper.py (2)
extract_url_params(29-54)is_token_error(25-26)
test/ably/rest/restauth_test.py (4)
ably/http/http.py (1)
Response(87-114)ably/rest/auth.py (2)
ably(299-300)Auth(29-437)ably/util/exceptions.py (1)
AblyAuthException(99-100)ably/types/mixins.py (1)
decode(46-126)
test/ably/rest/reststats_test.py (2)
ably/rest/rest.py (2)
http(108-109)stats(79-85)ably/http/paginatedresult.py (1)
PaginatedResult(44-111)
ably/types/tokenrequest.py (2)
ably/types/message.py (1)
timestamp(93-94)ably/types/presence.py (1)
timestamp(76-77)
ably/scripts/unasync.py (2)
ably/types/mixins.py (2)
encoding(35-36)encoding(39-43)ably/types/presence.py (1)
encoding(72-73)
test/ably/rest/encoders_test.py (1)
ably/util/crypto.py (2)
CipherParams(16-42)get_cipher(163-168)
ably/rest/auth.py (2)
ably/rest/rest.py (2)
options(112-113)AblyRest(18-145)ably/util/exceptions.py (3)
AblyException(9-84)AblyAuthException(99-100)from_dict(83-84)
test/ably/rest/restrequest_test.py (1)
test/ably/utils.py (3)
BaseAsyncTestCase(44-59)VaryByProtocolTestsMetaclass(120-157)dont_vary_protocol(160-162)
test/ably/rest/resthttp_test.py (2)
ably/rest/rest.py (1)
http(108-109)ably/http/http.py (2)
preferred_scheme(287-288)preferred_port(283-284)
test/ably/rest/restcapability_test.py (1)
test/ably/utils.py (3)
BaseAsyncTestCase(44-59)VaryByProtocolTestsMetaclass(120-157)dont_vary_protocol(160-162)
test/ably/realtime/realtimechannel_vcdiff_test.py (2)
ably/types/connectionstate.py (1)
ConnectionState(8-16)ably/types/options.py (1)
VCDiffDecoder(11-24)
test/ably/rest/restchannelstatus_test.py (1)
test/ably/utils.py (2)
BaseAsyncTestCase(44-59)VaryByProtocolTestsMetaclass(120-157)
ably/realtime/connectionmanager.py (5)
ably/transport/websockettransport.py (1)
ProtocolMessageAction(34-46)ably/types/connectiondetails.py (1)
ConnectionDetails(5-20)ably/types/connectionstate.py (2)
ConnectionEvent(19-28)ConnectionState(8-16)ably/util/exceptions.py (2)
AblyException(9-84)IncompatibleClientIdException(103-104)ably/util/helper.py (2)
Timer(57-71)is_token_error(25-26)
test/ably/rest/resttime_test.py (2)
test/ably/utils.py (3)
BaseAsyncTestCase(44-59)VaryByProtocolTestsMetaclass(120-157)dont_vary_protocol(160-162)ably/rest/rest.py (1)
time(88-92)
ably/rest/push.py (1)
ably/types/channelsubscription.py (4)
PushChannelSubscription(4-60)channel_subscriptions_response_processor(63-65)channels_response_processor(68-70)device_id(23-24)
ably/realtime/realtime_channel.py (3)
ably/rest/channel.py (8)
ably(145-146)Channel(21-176)Channels(179-231)cipher(157-158)name(149-150)get(184-195)options(161-162)options(169-176)ably/util/crypto.py (1)
CipherParams(16-42)ably/util/exceptions.py (2)
from_dict(83-84)AblyException(9-84)
ably/__init__.py (2)
ably/rest/rest.py (2)
auth(104-105)AblyRest(18-145)ably/util/exceptions.py (3)
AblyAuthException(99-100)AblyException(9-84)IncompatibleClientIdException(103-104)
🔇 Additional comments (95)
pyproject.toml (2)
90-91: Linting rules expansion well-documented and aligned with PR objectives.The extended rule set adds isort (I), pyupgrade (UP), bugbear (B), and comprehensions (C4) checks as stated in the PR description. The inline comment clearly documents which formatters/checkers are now enabled.
94-94: UP026 ignore appropriately justified for Python 3.7 compatibility.The
AsyncMockwas introduced inunittest.mockonly in Python 3.8. Since the project supports Python 3.7+ and includes themockpackage as a dev dependency (line 45) for backport support, suppressing UP026 is the correct approach to avoid breaking Python 3.7 compatibility.ably/types/options.py (2)
2-2: LGTM!The import reordering aligns with isort conventions and has no functional impact.
304-304: LGTM!The f-string conversion is correct and improves consistency with line 331, which already uses the same pattern.
ably/types/tokenrequest.py (1)
19-28: F-string change looks correct; only a tiny redundancy to be aware ofUsing
f"{self.timestamp or 0}"here is fine and preserves the intent of always including a numeric timestamp-like value in the MAC input, including whentimestampis falsy. Given the surroundingstr(x)comprehension, this value is now a string that then gets passed throughstr()again, which is harmless but slightly redundant.As long as
timestampis always an integer (or at least something whose string form is what you want in the signature), this change is behaviorally equivalent and safe. If there are any call sites that might pass non-integer numerics, it’s worth double‑checking that their string representation is indeed what you intend to sign over.ably/types/capability.py (1)
3-4: Modernized MutableMapping import looks goodSwitching to
from collections.abc import MutableMappingis the current recommended form and is behaviorally equivalent here. Typing imports remain correct.test/ably/realtime/realtimeresume_test.py (1)
1-4: Import grouping/spacing is fineThe added blank line cleanly separates stdlib (
asyncio) from third-party/project imports; no behavioral impact.ably/util/eventemitter.py (1)
1-5: Import formatting change is safeAdding the blank line between stdlib and third‑party imports is a pure formatting improvement with no effect on the
EventEmitterlogic.ably/types/channelstate.py (1)
1-6: Import order cleanup is appropriateReordering imports and adding the blank line before the project import improves readability without affecting
ChannelStateorChannelStateChangebehavior.ably/types/connectionstate.py (1)
1-6: Connection state imports reordering is fineThe slight import reordering keeps stdlib imports grouped and does not alter any of the enums or dataclass behavior.
test/ably/realtime/realtimeinit_test.py (1)
1-8: Import organization aligns with style goalsThe added blank lines and repositioning of
ConnectionStatealongside other Ably imports improve readability and have no impact on the tests’ behavior.ably/http/httputils.py (1)
39-46: Ably-Agent header f‑string is equivalent and clearerUsing an f‑string for the
Ably-Agentheader keeps the value identical while matching the modern formatting style introduced by the new linting rules.test/ably/rest/restcapability_test.py (1)
6-6: Consolidated test utilities import looks goodImporting
BaseAsyncTestCase,VaryByProtocolTestsMetaclass, anddont_vary_protocolfromtest.ably.utilskeeps the test setup consistent with other files; no behavior change.test/ably/utils.py (2)
9-17: Switch tounittest.mockis appropriateImporting
mockfrom the standard library (unittest.mock) avoids the extra third‑party dependency and should behave identically for the usage in this file.
187-200: Timeout re-raise message is clearer and context suppression is reasonableCatching
asyncio.TimeoutErrorand re-raising it with a clearer message while usingfrom Nonekeeps the exception type the same but produces cleaner tracebacks fromassert_waiter, which is useful in tests.test/ably/rest/restchannelstatus_test.py (1)
4-4: Standardized import of async test utilitiesUsing a single import from
test.ably.utilsforBaseAsyncTestCaseandVaryByProtocolTestsMetaclassmatches the rest of the suite and doesn’t change behavior.ably/types/presence.py (1)
80-83: F-string conversions preserve behaviorUsing f-strings for
member_keyand__base_pathkeeps the formatting identical to the old%-style while improving readability; no behavioral change.Also applies to: 118-120
test/ably/rest/restrequest_test.py (1)
9-24: Request path and seeded message formatting changes are equivalentThe consolidated utilities import is fine, and switching
self.pathplus the seededbodyvalues to f-strings keeps the generated paths and payloads identical (e.g.,'event0','lorem ipsum 0'), so tests remain valid.ably/scripts/unasync.py (1)
71-85: Encoding-aware reopen fixes potential decoding mismatchesReopening the file with
encoding=encodingafter detecting it viatokenize.detect_encodingensurestokenize_rtsees correctly decoded source and that the rewritten file is re-encoded with the original encoding. This is a solid robustness improvement for non-UTF-8 or non-default encoded files.ably/rest/channel.py (6)
3-18: LGTM! Import reorganization and logging setup.The import reordering and module-level logging setup follow Python best practices and align with the linting rules being added.
25-25: LGTM! F-string conversion is correct.The conversion from
.format()to f-string maintains the same functionality while improving readability.
50-50: LGTM! F-string conversion for message ID.The conversion maintains the same output format for idempotent message IDs.
60-62: LGTM! F-string conversion for exception message.The multi-line f-string correctly interpolates both client_id values in the error message.
86-86: LGTM! F-string conversion for TypeError.The conversion correctly includes the type representation in the error message.
139-139: LGTM! F-string conversion for status path.The simplified f-string is cleaner than the previous format-based approach.
test/ably/realtime/eventemitter_test.py (1)
1-2: LGTM! Import formatting improvement.The blank line after the asyncio import follows standard import grouping conventions.
test/ably/conftest.py (1)
3-4: LGTM! Import formatting improvement.The blank line separates stdlib imports from third-party imports, following standard import grouping conventions.
test/ably/realtime/realtimeauth_test.py (1)
3-7: LGTM! Added missing import and improved formatting.The explicit
urllib.parseimport is necessary for thequote_pluscalls on lines 145 and 483. The blank line follows standard import grouping conventions.test/ably/realtime/realtimechannel_vcdiff_test.py (1)
5-7: LGTM! Import reorganization.Moving these imports to the top of the file with other imports follows Python best practices and isort conventions.
ably/http/paginatedresult.py (2)
13-13: LGTM! F-string conversion for timestamp parameter.The conversion correctly formats the millisecond timestamp as a string.
36-36: LGTM! F-string conversion for limit parameter.The conversion correctly formats the limit value as a string.
test/ably/realtime/realtimechannel_test.py (1)
1-11: LGTM! Import reorganization and formatting.The import additions and reorganization follow standard conventions:
- Blank lines separate import groups (stdlib, third-party, local)
- All imported symbols are used in the test file
- No functional changes
ably/realtime/connection.py (4)
1-6: LGTM! Import organization and TYPE_CHECKING addition.The import reorganization and TYPE_CHECKING import follow Python best practices for avoiding circular imports.
44-44: LGTM! Type annotation modernization.The conversion from
Optional[AblyException]toAblyException | Nonefollows PEP 604 syntax and is functionally equivalent.
107-109: LGTM! Type annotation modernization.The conversion from
Optional[AblyException]toAblyException | Nonefollows PEP 604 syntax and maintains the same semantics.
120-121: LGTM! Type annotation modernization.The conversion from
Optional[ConnectionDetails]toConnectionDetails | Nonefollows PEP 604 syntax and is functionally equivalent.test/ably/realtime/realtimeconnection_test.py (1)
2-7: Import reordering looks goodCentralizing the
ConnectionEvent,ConnectionState, andDefaultsimports at the top is clean and keeps dependencies explicit; test behavior is unchanged.ably/realtime/realtime.py (1)
2-9: Logging and import tweaks are soundKeeping a module-level
logand groupingChannelswith the realtime imports is idiomatic and maintains existing behavior.test/ably/rest/resttoken_test.py (1)
4-13: Mock and Ably import consolidation is appropriateUsing
unittest.mock.patchand collapsing Ably/test utility imports improves consistency and readability without changing test behavior.test/ably/rest/reststats_test.py (1)
2-11: Stats test imports are well-structuredDatetime,
PaginatedResult, and test utility imports are now explicit and accurately reflect what the tests use; no behavior change.ably/types/authoptions.py (1)
37-39: Exception chaining change inset_keyis reasonableRaising
AblyException(..., 401, 40101) from Nonestill surfaces the same error to callers while hiding the internalValueErrortraceback, which matches how other Ably exceptions are cleaned up.ably/types/typedbuffer.py (1)
77-104: F-string error messages keep behavior intactSwitching the
ValueError/TypeErrormessages infrom_objanddecodeto f-strings is purely cosmetic; exception types, conditions, and content remain consistent.test/ably/rest/encoders_test.py (1)
5-18: Migration tounittest.mockis appropriateImporting
mockfromunittestand keeping AsyncMock version-gating preserves behavior while relying primarily on the stdlib mocking API; theget_cipherimport reordering is also benign.ably/transport/websockettransport.py (2)
2-24: WebSockets imports and typing improve compatibility (verify against target websockets versions)Using
ClientConnection/ top-levelconnectwhen available, falling back towebsockets.client.*, plus theadditional_headers→extra_headersTypeError fallback, is a sensible way to support websockets 15+ and earlier, and theTYPE_CHECKINGimport keeps theConnectionManagertype hint cheap at runtime. Please double-check this against the exact websockets versions you intend to support, to confirm both header keyword names and import locations remain valid.
95-99: Exception chaining on WebSocket connect failures is an improvementWrapping
(WebSocketException, socket.gaierror)in anAblyExceptionwhile both emitting'failed'and re-raising withraise exception from epreserves existing signaling and now also keeps the original cause in the traceback, which is useful for debugging.Please ensure callers (including any tests) don’t rely on
ws_connectswallowing these exceptions; they now surface as task exceptions (albeit consumed inon_ws_connect_done).test/ably/rest/resthttp_test.py (3)
4-5: LGTM!Import reorganization correctly groups standard library imports together (
unittest.mockandurllib.parse.urljoin), following isort conventions.
55-56: LGTM!The f-string conversion is correct and more readable than the previous
%formatting. The URL construction properly uses thepreferred_scheme,host, andpreferred_portproperties.
134-134: LGTM!Consistent f-string formatting for URL construction, matching the pattern used in
test_host_fallback.ably/http/http.py (4)
2-2: LGTM!Explicit
jsonimport is correct – it's used bydump_body()at line 141.
11-14: LGTM!Import reordering follows isort conventions with proper grouping.
191-194: Good fix for late-binding closure bug (B023).Capturing
retry_countas a default parameter ensures the closure uses the value from each loop iteration rather than the final value after the loop completes. This is an important bugbear fix that prevents subtle bugs in the retry logic.
196-197: LGTM!The f-string conversion is correct and consistent with the test file changes.
test/ably/rest/restpaginatedresult_test.py (1)
18-18: LGTM!The f-string conversion correctly produces valid JSON content. The
int(res)ensures the page number is rendered as a JSON number rather than a quoted string.ably/types/message.py (2)
5-5: LGTM!Import reordering follows alphabetical convention per isort.
21-21: LGTM!The f-string conversion provides the same error message with better readability. Including
type(value)in the error message aids debugging.ably/__init__.py (5)
1-2: LGTM!Adding the logging import for the NullHandler setup below.
6-6: LGTM!Import reordering per isort conventions.
12-12: LGTM!Exception imports are now alphabetically ordered.
15-16: LGTM!Adding a
NullHandlerto the library's logger follows Python logging best practices for libraries. This prevents "No handler found" warnings when the library is used without explicit logging configuration.
18-19: Thelib_versionconstant (2.1.2) is already synchronized betweenably/__init__.pyandpyproject.toml. The version is correctly used inHttpUtils.default_headers()for the Ably-Agent header. No version inconsistency exists in the codebase.ably/util/crypto.py (2)
6-6: LGTM!Import reordering within the try block follows alphabetical convention.
177-178: LGTM!The f-string conversion with implicit string continuation is correct and readable.
test/ably/rest/resttime_test.py (1)
7-7: LGTM! Clean modernization.The import reordering and f-string conversions improve code consistency without affecting functionality.
Also applies to: 27-27, 34-34
test/ably/rest/restpresence_test.py (1)
9-9: LGTM! Consistent formatting.The f-string conversion correctly mirrors the formatting used in the production code at
ably/types/presence.py.Also applies to: 72-72
test/ably/testapp.py (1)
3-3: LGTM! Import and formatting cleanup.The import reorganization and string formatting updates are consistent with the linting rules being applied.
Also applies to: 5-5, 13-13, 59-61
test/ably/rest/restpush_test.py (1)
8-17: LGTM! Idiomatic Python improvements.The import reorganization and use of
_for unused loop variables follow Python best practices and improve code clarity.Also applies to: 29-29, 256-256
test/ably/rest/restchannelhistory_test.py (1)
2-2: LGTM! Comprehensive f-string modernization.The systematic conversion from old-style
%formatting to f-strings improves readability and aligns with modern Python standards.Also applies to: 9-9, 62-62, 70-70, 77-77, 85-85, 136-136, 144-144, 151-151, 159-159, 166-166, 171-171, 176-176, 185-185, 192-192, 197-197, 202-202, 211-211, 218-218, 226-226, 234-234, 242-242, 249-249, 256-256, 264-264, 272-272, 278-278, 285-285, 293-293, 301-301, 308-308, 315-315, 323-323, 331-331
ably/util/exceptions.py (3)
4-4: LGTM! Clean f-string conversion.The f-string usage in
__str__is more readable and follows modern Python conventions.Also applies to: 23-25
46-46: LGTM! Proper exception context suppression.Using
from Nonecorrectly suppresses the original exception context when raising a cleanAblyException, avoiding confusing stack traces.Also applies to: 59-59
94-94: LGTM! Proper exception chaining.Preserving the exception chain with
from ein the decorator ensures full context is available for debugging while still wrapping inAblyException.ably/rest/rest.py (2)
6-6: LGTM! Import organization.The import reordering follows isort conventions for consistent import structure.
Also applies to: 13-13
64-65: LGTM! Cleaner attribute checking.Using
hasattris more explicit and idiomatic than catchingAttributeError, improving code readability.ably/types/mixins.py (2)
104-104: LGTM! Proper exception chaining.Preserving the original exception with
from eensures VCDiff decode failures retain their full error context for debugging.
106-106: LGTM! F-string modernization.The f-string conversions are correct and improve code readability.
Also applies to: 119-119
test/ably/rest/restcrypto_test.py (3)
46-47: LGTM! F-string conversions are correct.The log statements have been properly modernized to use f-strings.
Also applies to: 78-78
77-78: Dict comprehension is cleaner.Good conversion from
dict((m.name, m.data) for m in messages)to{m.name: m.data for m in messages}.
207-207: LGTM! Simplified open() call.Removing the redundant
'r'mode parameter is correct since it's the default.test/ably/rest/restauth_test.py (3)
119-120: LGTM! Bearer token assertion is correct.The f-string formatting for the Bearer authorization header matches the implementation in
ably/rest/auth.py.
222-222: LGTM! F-string for assertion message.Clean formatting for the assertion error message.
492-492: LGTM! F-strings for URL construction.URL construction using f-strings is cleaner and more readable.
Also applies to: 521-521
test/ably/rest/restchannelpublish_test.py (4)
59-60: LGTM! Dict comprehension and f-string conversions.Clean modernization of the message contents mapping and logging.
85-85: LGTM! F-strings for Message construction.Clean f-string usage for generating message names.
Also applies to: 104-104
405-405: Good use of default parameter values in closures.Adding
encoding=encoding, msg_data=msg_dataandexpected_value=expected_value, expected_type=expected_typeas default parameters ensures the closure captures the current loop iteration values rather than the final values. This is the correct pattern for closures inside loops.Also applies to: 421-421
497-497: LGTM! Set comprehension.Using
{case.snake_to_camel(x) for x in data.keys()}is cleaner than the previous approach.test/ably/rest/restinit_test.py (2)
1-2: LGTM! Import consolidation.Imports are properly reorganized.
Also applies to: 6-6, 10-10
78-78: LGTM! F-string conversions for assertion messages.The f-string formatting for error messages improves readability.
Also applies to: 83-83, 128-128, 140-140, 184-184, 190-190
ably/realtime/realtime_channel.py (4)
37-37: LGTM! PEP 604 union syntax for type hints.Using
CipherParams | Noneinstead ofOptional[CipherParams]aligns with modern Python typing.
115-128: LGTM! Type annotations for RealtimeChannel fields.The field type annotations using PEP 604 unions are correct and improve code clarity.
608-608: LGTM! Method signature with union type.The
getmethod signature properly usesChannelOptions | None.
62-67: CipherParams is hashable by default in Python 3.
CipherParamsdoes not implement__eq__or__hash__. In Python 3, when a class doesn't define__eq__, it remains hashable using the default identity-based__hash__method. Therefore,self.__ciphercan be safely hashed without raising aTypeError.However, there is a potential issue on line 66:
tuple(sorted(self.__params.items()))assumes all dictionary values are comparable. While the type hint indicatesdict[str, str], the code lacks runtime validation, so passing non-comparable values could raise aTypeErrorduring sorting.Likely an incorrect or invalid review comment.
ably/rest/auth.py (5)
34-34: LGTM! PEP 604 union type for constructor parameter.Using
AblyRest | AblyRealtimeis cleaner thanUnion[AblyRest, AblyRealtime].
46-49: LGTM! Field type annotations.The field type annotations using PEP 604 unions improve type safety and readability.
59-61: LGTM! F-string for credential construction.The f-string for constructing the basic key is cleaner than string concatenation.
189-189: Good use of exception chaining.Using
raise ... from epreserves the original exception chain, which is helpful for debugging. This follows Python best practices (PEP 3134).Also applies to: 213-213
372-382: LGTM! F-strings for Authorization headers.The f-string formatting for Basic and Bearer authorization headers is correct and consistent with the test expectations.
ttypic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Adds isort, pyupgrade, bugbear, and comprehensions linting rulesets to ruff checks
Summary by CodeRabbit
Release Notes
New Features
api_versionandlib_versionmodule constants for version reference.Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.