Skip to content

Commit 67ac2a9

Browse files
committed
docs: code review status updates
1 parent 142bcab commit 67ac2a9

1 file changed

Lines changed: 26 additions & 0 deletions

File tree

docs/design/V3_CODE_REVIEW_v3.0_to_v3.2.md

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,8 @@ The indirection added complexity without benefit.
3434

3535
### Concern: `response.py` is now a God-module
3636

37+
**Status:** Partially fixes, code duplication have been dealt with and squashd together with earlier work on this.
38+
3739
`response.py` grew from ~200 to ~900 lines and now contains result dataclasses, six XML
3840
parse functions, *and* the `DAVResponse` class with its own parse path. The file itself
3941
notes:
@@ -55,6 +57,8 @@ version.
5557

5658
### Concern: `base_client.py` XML builders are `@staticmethod`s on a class
5759

60+
**Status:** Ignored as for now.
61+
5862
The `_build_propfind_body`, `_build_calendar_query_body`, etc. methods are `@staticmethod`
5963
on `BaseDAVClient`. They do not use `self` or `cls`; they could be module-level functions.
6064
Placing them on the class forces any caller to either hold a client reference or write
@@ -71,6 +75,8 @@ across the codebase. The `_value_or_coroutine` hook for cache hits is a clever
7175

7276
### Major concern: `is_async_client` uses a string class-name comparison
7377

78+
**Status:** Fixed
79+
7480
```python
7581
# davobject.py:110
7682
return type(self.client).__name__ == "AsyncDAVClient"
@@ -83,6 +89,8 @@ should use `isinstance()` or a class-level flag attribute.
8389

8490
### Concern: dual-mode return types are misleading to type checkers
8591

92+
**Status:** Ignored as for now.
93+
8694
Methods like `get_calendars()` are typed as `list[Calendar] | Coroutine[...]`. In
8795
practice sync callers get a list and async callers get a coroutine — but nothing in the
8896
type system enforces that the caller actually awaits it. A sync caller who accidentally
@@ -92,6 +100,8 @@ proper solution in v4.0.
92100

93101
### Potential bug: `freebusy_request()` passes an unawaited coroutine
94102

103+
**Status:** **Needs attention in 3.2.1**
104+
95105
```python
96106
# collection.py
97107
outbox = self.schedule_outbox() # returns a coroutine in async mode, not an outbox
@@ -107,6 +117,8 @@ variable named `outbox` holds a coroutine. The inline comment acknowledges this
107117

108118
### Bug: `_async_complete` with RRULE silently drops `save()`
109119

120+
**Status:** **Needs attention in 3.2.1**
121+
110122
```python
111123
# calendarobjectresource.py — comment in _async_complete:
112124
# _complete_recurring_* methods are sync-only for now; they internally
@@ -131,6 +143,8 @@ be a `raise NotImplementedError(...)` with a clear message rather than a silent
131143

132144
### Concern: bare `assert` in `_parse_scheduling_response_objects()`
133145

146+
**Status:** Fixed
147+
134148
```python
135149
assert self.tree.tag == cdav.ScheduleResponse.tag
136150
assert response.tag == cdav.Response.tag
@@ -141,6 +155,8 @@ Use `error.assert_()` or explicit `if … raise` like the rest of the codebase d
141155

142156
### Concern: repeated ETag / Schedule-Tag update block
143157

158+
**Status:** Should be fixed, perhaps in 3.2.1. This code was hand-written by the guy who hates duplicated code.
159+
144160
```python
145161
## consider refactoring - this is repeated many places now
146162
if "Etag" in r.headers:
@@ -164,6 +180,8 @@ model for structured parse results.
164180

165181
### Concern: `_element_to_value` fallback returns a raw lxml element
166182

183+
**Status:** Ignored as for now. Should consider this.
184+
167185
```python
168186
# end of _element_to_value()
169187
return elem # returns an lxml _Element as a "value"
@@ -174,6 +192,8 @@ expecting strings or lists of strings. This path should at minimum log a warnin
174192

175193
### Concern: `_strip_to_multistatus` return type is opaque
176194

195+
**Status:** Ignored as for now. Should consider this.
196+
177197
```python
178198
def _strip_to_multistatus(tree: _Element) -> "_Element | list[_Element]":
179199
```
@@ -196,6 +216,8 @@ future maintainers.
196216

197217
### Concern: `_resolve_properties` dead-code path
198218

219+
**Status:** Ignored as for now. Should consider this.
220+
199221
```python
200222
error.assert_(False)
201223
return {} # newly added
@@ -218,6 +240,8 @@ if-chain.
218240

219241
### Minor: `_ConfiguredServer.start()` is a no-op, `is_accessible()` ignores reachability
220242

243+
**Status:** Ignored as for now. Should consider this.
244+
221245
```python
222246
def start(self) -> None:
223247
self._started = True # external — assumed already running
@@ -239,6 +263,8 @@ etc. brought in from `operations/calendarset_ops.py` are now module-level helper
239263

240264
### Concern: `_extract_calendar_id_from_url` swallows all exceptions
241265

266+
**Status:** Ignored as for now. Should consider this.
267+
242268
```python
243269
except Exception:
244270
log.error(f"Calendar has unexpected url {url}")

0 commit comments

Comments
 (0)