Skip to content

review: CLAUDE.md#24

Merged
qlrd merged 2 commits into
docs/claude-mdfrom
docs/claude-md-review
Mar 26, 2026
Merged

review: CLAUDE.md#24
qlrd merged 2 commits into
docs/claude-mdfrom
docs/claude-md-review

Conversation

@qlrd

@qlrd qlrd commented Mar 26, 2026

Copy link
Copy Markdown
Owner
  • Review docs/claude-md changes;

@qlrd qlrd requested a review from Copilot March 26, 2026 17:53

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review any files in this pull request.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@qlrd

qlrd commented Mar 26, 2026

Copy link
Copy Markdown
Owner Author

Review of Copilot comments from selfcustody#229 (CLAUDE.md):

Comment 1 — "Atomic commits" sentence fragment:
cACK. Nit, but the phrasing is a project convention. Could be smoothed out.

Comment 2 — Conflicting line-length (Black 88 vs Pylint 100):
ACK. This is a valid point — should clarify that Black enforces 88 for code, Pylint allows up to 100 as a lenient upper bound for non-Black-managed cases.

Comment 3 — pytest vs unittest.TestCase ambiguity:
ACK. Worth clarifying that the repo uses pytest as the runner, and some tests use unittest.TestCase-style classes.

Comment 4 — Example with literal filename.py:
NACK. This is an intentional project convention per AGENT.md — every Python file has a docstring with the filename and brief description. Not a placeholder issue.

@copilot comments 2 and 3 are valid and should be addressed. Comment 4 follows the project's AGENT.md license header convention intentionally.

Copilot AI commented Mar 26, 2026

Copy link
Copy Markdown

Review of Copilot comments from selfcustody#229 (CLAUDE.md):

Comment 1 — "Atomic commits" sentence fragment:...

Addressed comments 2 and 3 in ed53f70:

  • Comment 2: Updated the Code style section to explicitly state that Black enforces 88-char line length for code, while Pylint's max-line-length of 100 is a lenient upper bound for non-Black-managed cases.
  • Comment 3: Updated the Testing section to clarify that pytest is the test runner, and some test classes use unittest.TestCase-style structure.

Comment 4 left as-is per your NACK.

@qlrd

qlrd commented Mar 26, 2026

Copy link
Copy Markdown
Owner Author

tACK ed53f70

Copilot addressed comments 2 and 3. Comment 4 correctly NACKed per AGENT.md convention.

@copilot all resolved.

@qlrd qlrd requested a review from Copilot March 26, 2026 18:57
@qlrd qlrd merged commit 1e0bb31 into docs/claude-md Mar 26, 2026
5 checks passed

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread CLAUDE.md

- **Formatter:** Black (max line length follows Black defaults)
- **Linter:** Pylint (max line: 100, max statements: 50, see `.pylint/src`)
- **Formatter:** Black (enforces 88-char line length for all code)

Copilot AI Mar 26, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The statement that Black "enforces 88-char line length for all code" is a bit too absolute: Black’s default line length is 88, but it can still emit longer lines (e.g., long strings/URLs, some comments) and won’t always reflow everything under 88. Consider rephrasing to "Black (default line length 88)" or similar to avoid an incorrect guarantee.

Suggested change
- **Formatter:** Black (enforces 88-char line length for all code)
- **Formatter:** Black (default line length 88)

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK. Good catch — Black's default is 88 but it doesn't guarantee all lines stay under 88.

Comment thread CLAUDE.md
- **Formatter:** Black (enforces 88-char line length for all code)
- **Linter:** Pylint (max line: 100 as a lenient upper bound for non-Black-managed cases,
max statements: 50, see `.pylint/src`)
- **Python:** >=3.12.0 required

Copilot AI Mar 26, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doc says Python >=3.12.0 is required, but the project’s declared Poetry constraint is ">3.10,<=3.14" (pyproject.toml). Either tighten the Poetry constraint to match the stated requirement, or relax/update this line to match what the project actually supports/tests.

Suggested change
- **Python:** >=3.12.0 required
- **Python:** >3.10,<=3.14 (see `pyproject.toml` for the authoritative version constraint)

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK. The CLAUDE.md should reference pyproject.toml as the source of truth for the Python version constraint.

@qlrd qlrd deleted the docs/claude-md-review branch March 26, 2026 19:00
@qlrd

qlrd commented Mar 26, 2026

Copy link
Copy Markdown
Owner Author

@copilot please apply the following fixes:

  1. Change "enforces 88-char line length for all code" to "default line length 88" for Black;
  2. Update the Python version line to reference pyproject.toml as the source of truth instead of hardcoding >=3.12.0;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants