Skip to content

Commit 5f17d9a

Browse files
Copilotnathanjmcdougall
Copilot
and
nathanjmcdougall
authored
refactor: hoist shared if-else logic; document principle in skill (#1890)
* Initial plan * refactor: hoist shared tick_print in remove_deps_from_group; add if-else nesting skill guidance Agent-Logs-Url: https://github.com/usethis-python/usethis-python/sessions/666ce694-85bc-4b4e-9d64-4dd29447b897 Co-authored-by: nathanjmcdougall <18602289+nathanjmcdougall@users.noreply.github.com> * Fix newline issue Also fix priority config issues in prek --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: nathanjmcdougall <18602289+nathanjmcdougall@users.noreply.github.com> Co-authored-by: Nathan McDougall <nathan.j.mcdougall@gmail.com>
1 parent 9c6a728 commit 5f17d9a

3 files changed

Lines changed: 67 additions & 19 deletions

File tree

‎.agents/skills/usethis-python-code/SKILL.md‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -207,6 +207,54 @@ def _format_version(v: str) -> str:
207207
- **Adding helpers at the top of the file.** It is tempting to place a new helper near the top, before any existing function. Always scroll down to find the caller first, then add the helper below it.
208208
- **Placing a helper above the function that introduces it.** Even if a helper is only a few lines long, it should still follow its caller so the intent is clear before the detail.
209209

210+
## Structuring if-else to avoid duplicated logic
211+
212+
When an outer if-else branches on an enum (or similar value), and a shared guard condition or action applies to a _group_ of those branches, check the guard once at the outer level using a combined membership test before branching on the individual values.
213+
214+
If the same string literal or logic block appears in two sibling branches of an if-else, that is a signal to restructure: hoist the shared condition or action to the outer level, and move the distinguishing logic to the inner level.
215+
216+
### Procedure
217+
218+
1. Identify sibling branches of an if-elif chain that contain identical statements.
219+
2. Extract those identical statements by replacing the outer per-value tests with a combined membership check: `if value in (A, B):`.
220+
3. Inside that combined block, nest the per-value branching for any logic that genuinely differs between the values.
221+
4. Verify the `else: assert_never(...)` guard is still present at every level where the value is dispatched.
222+
223+
### Example
224+
225+
```python
226+
# Bad: same tick_print duplicated in two sibling branches
227+
if backend is BackendEnum.uv:
228+
tick_print("Doing work in 'pyproject.toml'.")
229+
do_uv_work()
230+
elif backend is BackendEnum.poetry:
231+
tick_print("Doing work in 'pyproject.toml'.") # identical
232+
do_poetry_work()
233+
elif backend is BackendEnum.none:
234+
instruct_print("Do the work manually.")
235+
else:
236+
assert_never(backend)
237+
238+
# Good: shared message hoisted; distinguishing logic nested inside
239+
if backend in (BackendEnum.uv, BackendEnum.poetry):
240+
tick_print("Doing work in 'pyproject.toml'.")
241+
if backend is BackendEnum.uv:
242+
do_uv_work()
243+
elif backend is BackendEnum.poetry:
244+
do_poetry_work()
245+
else:
246+
assert_never(backend)
247+
elif backend is BackendEnum.none:
248+
instruct_print("Do the work manually.")
249+
else:
250+
assert_never(backend)
251+
```
252+
253+
### Common mistakes
254+
255+
- **Nesting a shared guard inside each branch independently.** When a guard condition (such as "is git available?") applies equally to multiple branches, nesting it separately inside each branch leads to duplicated code that is hard to keep in sync. Hoist the guard to the outer level instead.
256+
- **Overlooking the inner `assert_never`.** After introducing the combined membership check, the inner if-elif still needs its own `else: assert_never(backend)` to preserve exhaustiveness checking.
257+
210258
## Marking derived properties `@final`
211259

212260
When a property derives its return value entirely from a single source of truth (e.g. `return self.meta.name`), mark it `@final` to prevent subclasses from overriding it with independent logic.

‎.pre-commit-config.yaml‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -48,12 +48,6 @@ repos:
4848
- id: ruff-check
4949
args: [--fix]
5050
priority: 0
51-
- repo: https://github.com/DavidAnson/markdownlint-cli2
52-
rev: v0.22.0
53-
hooks:
54-
- id: markdownlint-cli2
55-
args: ["--fix"]
56-
priority: 0
5751
- repo: https://github.com/codespell-project/codespell
5852
rev: v2.4.1
5953
hooks:
@@ -199,12 +193,18 @@ repos:
199193
always_run: true
200194
pass_filenames: false
201195
priority: 1
196+
- repo: https://github.com/DavidAnson/markdownlint-cli2
197+
rev: v0.22.0
198+
hooks:
199+
- id: markdownlint-cli2
200+
args: ["--fix"]
201+
priority: 2
202202
- repo: https://github.com/rbubley/mirrors-prettier
203203
rev: v3.8.1
204204
hooks:
205205
- id: prettier
206206
types_or: [markdown]
207-
priority: 2
207+
priority: 3
208208
- repo: local
209209
hooks:
210210
- id: deptry
@@ -213,22 +213,22 @@ repos:
213213
language: system
214214
always_run: true
215215
pass_filenames: false
216-
priority: 3
216+
priority: 4
217217
- id: import-linter
218218
name: import-linter
219219
entry: uv run --frozen --offline lint-imports
220220
language: system
221221
always_run: true
222222
pass_filenames: false
223-
priority: 3
223+
priority: 4
224224
- id: ty
225225
name: ty
226226
entry: uv run --frozen --offline ty check
227227
language: system
228228
types_or: [python, pyi]
229229
always_run: true
230230
require_serial: true
231-
priority: 3
231+
priority: 4
232232
default_stages:
233233
- pre-commit
234234
- pre-push

‎src/usethis/_deps.py‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -183,18 +183,18 @@ def remove_deps_from_group(deps: list[Dependency], group: str) -> None:
183183
ies = "y" if len(_deps) == 1 else "ies"
184184
backend = get_backend()
185185

186-
if backend is BackendEnum.uv:
187-
tick_print(
188-
f"Removing dependenc{ies} {deps_str} from the '{group}' group in 'pyproject.toml'."
189-
)
190-
for dep in _deps:
191-
remove_dep_from_group_via_uv(dep, group)
192-
elif backend is BackendEnum.poetry:
186+
if backend in (BackendEnum.uv, BackendEnum.poetry):
193187
tick_print(
194188
f"Removing dependenc{ies} {deps_str} from the '{group}' group in 'pyproject.toml'."
195189
)
196-
for dep in _deps:
197-
remove_dep_from_group_via_poetry(dep, group)
190+
if backend is BackendEnum.uv:
191+
for dep in _deps:
192+
remove_dep_from_group_via_uv(dep, group)
193+
elif backend is BackendEnum.poetry:
194+
for dep in _deps:
195+
remove_dep_from_group_via_poetry(dep, group)
196+
else:
197+
assert_never(backend)
198198
elif backend is BackendEnum.none:
199199
instruct_print(f"Remove the {group} dependenc{ies} {deps_str}.")
200200
else:

0 commit comments

Comments
 (0)