diff --git a/.github/RESOLVING_PR_COMMENTS.md b/.github/RESOLVING_PR_COMMENTS.md new file mode 100644 index 0000000..a1b5d4d --- /dev/null +++ b/.github/RESOLVING_PR_COMMENTS.md @@ -0,0 +1,376 @@ +# How to Resolve GitHub Review Comments + +This guide documents how to mark review comment conversations as resolved after addressing reviewer feedback in pull requests. + +## Quick Reference + +```bash +# 1. Get all review threads for PR #74 +gh api graphql -f query=' +query { + repository(owner: "eman", name: "nwp500-python") { + pullRequest(number: 74) { + reviewThreads(first: 10) { + nodes { + id + path + line + isResolved + } + } + } + } +} +' | jq '.data.repository.pullRequest.reviewThreads.nodes' + +# 2. Resolve a specific thread +gh api graphql -f query=' +mutation { + resolveReviewThread(input: {threadId: "PRRT_kwDOP_hNvM5ukIVT"}) { + thread { isResolved } + } +} +' +``` + +## Why Resolve Comments? + +- **Clarity**: Shows reviewers their feedback has been acted upon +- **Workflow**: Prevents reviewers from re-reading already-addressed comments +- **Signal Readiness**: Indicates PR is ready for another review pass +- **Clean PR Interface**: Makes GitHub's PR conversation cleaner and easier to follow + +## Workflow: Address and Resolve + +### 1. Address the Comment + +Make the code changes requested by the reviewer: +- Fix bugs +- Update documentation +- Refactor code +- Add tests + +**Example:** +```bash +# Edit file based on comment +nano src/nwp500/converters.py + +# Commit the change +git add src/nwp500/converters.py +git commit -m "Fix div_10 converter to handle string inputs" + +# Push to remote +git push +``` + +### 2. Verify Tests Pass + +Run all validation before marking comments as resolved: + +```bash +# Run tests +pytest --ignore=tests/test_mqtt_hypothesis.py + +# Run linting +make ci-lint + +# Run type checking +python3 -m mypy src/nwp500 --config-file pyproject.toml +``` + +All must pass before proceeding. + +### 3. Get Review Thread IDs + +Query GraphQL to find threads that need resolving: + +```bash +gh api graphql -f query=' +query { + repository(owner: "eman", name: "nwp500-python") { + pullRequest(number: 74) { + reviewThreads(first: 10) { + nodes { + id + path + line + isResolved + } + } + } + } +} +' | jq '.data.repository.pullRequest.reviewThreads.nodes' +``` + +**Output example:** +```json +[ + { + "id": "PRRT_kwDOP_hNvM5ukIVT", + "path": "src/nwp500/converters.py", + "line": 125, + "isResolved": false + }, + { + "id": "PRRT_kwDOP_hNvM5ukIVo", + "path": "tests/test_model_converters.py", + "line": 212, + "isResolved": false + } +] +``` + +### 4. Identify Which Threads to Resolve + +Cross-reference the output with: +1. Which file you modified +2. Which line the comment was on (approximately) +3. Whether `isResolved` is `false` + +### 5. Resolve the Threads + +For each thread you addressed: + +```bash +gh api graphql -f query=' +mutation { + resolveReviewThread(input: {threadId: "PRRT_kwDOP_hNvM5ukIVT"}) { + thread { + isResolved + } + } +} +' +``` + +Success response: +```json +{ + "data": { + "resolveReviewThread": { + "thread": { + "isResolved": true + } + } + } +} +``` + +### 6. Verify All Are Resolved + +Re-run the query from Step 3 to confirm all addressed threads now show `"isResolved": true`: + +```bash +gh api graphql -f query=' +query { + repository(owner: "eman", name: "nwp500-python") { + pullRequest(number: 74) { + reviewThreads(first: 10) { + nodes { + path + isResolved + } + } + } + } +} +' | jq '.data.repository.pullRequest.reviewThreads.nodes' +``` + +## Batch Resolving Multiple Threads + +If you have many threads to resolve, use a loop: + +```bash +#!/bin/bash + +# Define thread IDs +THREAD_IDS=( + "PRRT_kwDOP_hNvM5ukIVT" + "PRRT_kwDOP_hNvM5ukIVo" + "PRRT_kwDOP_hNvM5ukIVx" +) + +# Resolve each one +for thread_id in "${THREAD_IDS[@]}"; do + gh api graphql -f query=" + mutation { + resolveReviewThread(input: {threadId: \"$thread_id\"}) { + thread { isResolved } + } + } + " && echo "✓ $thread_id resolved" +done + +echo "All threads resolved!" +``` + +Or as a one-liner: + +```bash +for id in PRRT_kwDOP_hNvM5ukIVT PRRT_kwDOP_hNvM5ukIVo PRRT_kwDOP_hNvM5ukIVx; do + gh api graphql -f query="mutation { resolveReviewThread(input: {threadId: \"$id\"}) { thread { isResolved } } }" && echo "✓ $id resolved" +done +``` + +## Shell Function (Optional) + +Add this to your shell profile (`~/.bashrc`, `~/.zshrc`, etc.): + +```bash +# Resolve a single GitHub PR review thread +resolve-pr-thread() { + local thread_id="$1" + if [[ -z "$thread_id" ]]; then + echo "Usage: resolve-pr-thread THREAD_ID" + echo "Example: resolve-pr-thread PRRT_kwDOP_hNvM5ukIVT" + return 1 + fi + + gh api graphql -f query=" + mutation { + resolveReviewThread(input: {threadId: \"$thread_id\"}) { + thread { isResolved } + } + } + " | jq '.data.resolveReviewThread.thread.isResolved' +} + +# Get all unresolved threads for a PR +pr-threads() { + local pr_num="${1:?PR number required}" + gh api graphql -f query=" + query { + repository(owner: \"eman\", name: \"nwp500-python\") { + pullRequest(number: $pr_num) { + reviewThreads(first: 10) { + nodes { + id + path + line + isResolved + } + } + } + } + } + " | jq '.data.repository.pullRequest.reviewThreads.nodes' +} +``` + +Usage: +```bash +pr-threads 74 # List all threads for PR #74 +resolve-pr-thread PRRT_kwDOP_hNvM5ukIVT # Resolve one thread +``` + +## Special Cases + +### Unresolving a Thread + +If a reviewer asks for changes after you marked it resolved, unresolve it: + +```bash +gh api graphql -f query=' +mutation { + unresolveReviewThread(input: {threadId: "PRRT_kwDOP_hNvM5ukIVT"}) { + thread { + isResolved + } + } +} +' +``` + +### Force-Pushed Commits + +When you amend and force-push commits: +1. Old review threads remain resolvable +2. Thread IDs don't change +3. Line numbers may be different in new commits +4. Comments still point to old code but threads can be resolved +5. This is normal GitHub behavior - resolve all threads once your changes are complete + +### Multiple Changes to Same File + +If a reviewer left multiple comments on the same file and you addressed them all: +1. Make all changes to the file +2. Commit and push +3. Get all thread IDs for that file +4. Resolve each one individually + +## Troubleshooting + +### "Not Found" Error + +**Problem:** +``` +{ + "message": "Not Found", + "documentation_url": "https://docs.github.com/rest", + "status": 404 +} +``` + +**Solutions:** +- Verify PR number is correct +- Check you have access to the repository +- Verify `gh auth status` shows you're authenticated +- Try running `gh auth login` again + +### No Threads Returned + +**Problem:** +``` +{ + "data": { + "repository": { + "pullRequest": { + "reviewThreads": { + "nodes": [] + } + } + } + } +} +``` + +**Solutions:** +- Verify PR number is correct +- The PR may have only general comments (not review comments) +- Try increasing `first: 10` to `first: 100` if there are many threads +- Verify reviewers left inline code comments, not just PR comments + +### Thread Won't Resolve + +**Problem:** +Mutation succeeds but `isResolved` still returns `false` on next check + +**Solutions:** +- Wait a moment and query again (GitHub API may have delay) +- Verify you're using the exact same thread ID +- Check that you have write permissions on the repository +- Try running `gh auth refresh` to refresh your token + +### Can't Find Thread ID for Comment I Fixed + +**Problem:** +You fixed the code but can't find the matching thread ID + +**Possible Causes:** +1. The comment was a general PR comment, not an inline code comment (can't be resolved) +2. The thread was already resolved by someone else +3. You're looking at the wrong PR number +4. The comment was deleted by the reviewer + +**Solution:** +Go to the PR on GitHub.com and manually verify the comment still exists and is an inline review comment (not a general comment). + +## References + +- [GitHub GraphQL API: resolveReviewThread](https://docs.github.com/en/graphql/reference/mutations#resolvereviewthread) +- [GitHub GraphQL API: unresolveReviewThread](https://docs.github.com/en/graphql/reference/mutations#unresolvereviewthread) +- [GitHub GraphQL API: Review Threads](https://docs.github.com/en/graphql/reference/objects#reviewthread) +- [GitHub CLI: gh api](https://cli.github.com/manual/gh_api) +- [GitHub: Review conversations](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/commenting-on-a-pull-request#about-pull-request-reviews) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index d097d4d..a6dfeed 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -54,6 +54,71 @@ When working on pull requests, use the GitHub CLI to access review comments: This ensures you can address all feedback from code reviewers systematically. +#### Marking Conversations as Resolved + +After addressing a review comment, mark the conversation as resolved to signal reviewers that the feedback has been acted upon. + +**Step 1: Get review thread IDs** +```bash +gh api graphql -f query=' +query { + repository(owner: "eman", name: "nwp500-python") { + pullRequest(number: 74) { + reviewThreads(first: 10) { + nodes { + id + path + line + isResolved + } + } + } + } +} +' +``` + +Replace `74` with the actual PR number. This returns thread IDs like `PRRT_kwDOP_hNvM5ukIVT`. + +**Step 2: Resolve a thread** +```bash +gh api graphql -f query=' +mutation { + resolveReviewThread(input: {threadId: "PRRT_kwDOP_hNvM5ukIVT"}) { + thread { + isResolved + } + } +} +' +``` + +Replace the thread ID with the one from Step 1. Success response shows `"isResolved": true`. + +**Step 3: Resolve multiple threads efficiently** +```bash +for id in PRRT_kwDOP_hNvM5ukIVT PRRT_kwDOP_hNvM5ukIVo PRRT_kwDOP_hNvM5ukIVx; do + gh api graphql -f query=" + mutation { + resolveReviewThread(input: {threadId: \"$id\"}) { + thread { isResolved } + } + } + " && echo "✓ $id resolved" +done +``` + +**Step 4: Verify all are resolved** +Re-run Step 1 query and confirm all addressed threads show `"isResolved": true`. + +**Important Notes:** +- Only inline code review comments can be resolved (not general PR comments) +- Conversations remain resolvable even after force-pushing commits +- When commits are amended and force-pushed, old thread IDs remain valid +- If you need to reopen a resolved thread, use `unresolveReviewThread` mutation instead + +See detailed instructions at `.github/copilot-instructions.md` for more examples and troubleshooting. + ### Before Committing Changes Always run these checks before finalizing changes to ensure your code will pass CI: 1. **Linting**: `make ci-lint` - Ensures code style matches CI requirements diff --git a/src/nwp500/converters.py b/src/nwp500/converters.py index bcc92df..e0e2c0d 100644 --- a/src/nwp500/converters.py +++ b/src/nwp500/converters.py @@ -122,7 +122,7 @@ def div_10(value: Any) -> float: """ if isinstance(value, (int, float)): return float(value) / 10.0 - return float(value) + return float(value) / 10.0 def mul_10(value: Any) -> float: @@ -145,7 +145,7 @@ def mul_10(value: Any) -> float: """ if isinstance(value, (int, float)): return float(value) * 10.0 - return float(value) + return float(value) * 10.0 def enum_validator(enum_class: type[Any]) -> Callable[[Any], Any]: diff --git a/src/nwp500/encoding.py b/src/nwp500/encoding.py index 6c087fc..f80c3ae 100644 --- a/src/nwp500/encoding.py +++ b/src/nwp500/encoding.py @@ -297,14 +297,14 @@ def decode_reservation_hex(hex_string: str) -> list[dict[str, int]]: for i in range(0, len(data), 6): chunk = data[i : i + 6] - # Skip empty entries (all zeros) - if all(b == 0 for b in chunk): - continue - # Ensure we have a full 6-byte entry if len(chunk) != 6: break + # Skip empty entries (all zeros) + if all(b == 0 for b in chunk): + continue + reservations.append( { "enable": chunk[0], diff --git a/src/nwp500/factory.py b/src/nwp500/factory.py index b543000..9edf6f3 100644 --- a/src/nwp500/factory.py +++ b/src/nwp500/factory.py @@ -73,7 +73,12 @@ async def create_navien_clients( auth_client = NavienAuthClient(email, password) # Authenticate and enter context manager - await auth_client.__aenter__() + try: + await auth_client.__aenter__() + except BaseException: + # Ensure session is cleaned up if authentication fails + await auth_client.__aexit__(None, None, None) + raise # Create API and MQTT clients that share the session api_client = NavienAPIClient(auth_client=auth_client) diff --git a/src/nwp500/mqtt/command_queue.py b/src/nwp500/mqtt/command_queue.py index aa933ab..dabfa2a 100644 --- a/src/nwp500/mqtt/command_queue.py +++ b/src/nwp500/mqtt/command_queue.py @@ -100,8 +100,8 @@ def enqueue( self._queue.put_nowait(command) _logger.info(f"Queued command (queue size: {self._queue.qsize()})") except asyncio.QueueFull: - # Should not happen since we checked/cleared above _logger.error("Failed to enqueue command - queue unexpectedly full") + raise async def send_all( self, diff --git a/src/nwp500/mqtt/subscriptions.py b/src/nwp500/mqtt/subscriptions.py index 632cee9..fb50bfd 100644 --- a/src/nwp500/mqtt/subscriptions.py +++ b/src/nwp500/mqtt/subscriptions.py @@ -200,7 +200,8 @@ async def subscribe( self._subscriptions[topic] = qos if topic not in self._message_handlers: self._message_handlers[topic] = [] - self._message_handlers[topic].append(callback) + if callback not in self._message_handlers[topic]: + self._message_handlers[topic].append(callback) return int(packet_id) diff --git a/src/nwp500/temperature.py b/src/nwp500/temperature.py index ef43501..78ce822 100644 --- a/src/nwp500/temperature.py +++ b/src/nwp500/temperature.py @@ -299,8 +299,8 @@ def to_fahrenheit_with_formula( return float(math.floor(fahrenheit_value)) case _: return float(math.ceil(fahrenheit_value)) - case TempFormulaType.STANDARD: - # Standard Rounding (default) + case _: + # Standard Rounding (default for STANDARD and any future types) return round(fahrenheit_value) @classmethod diff --git a/src/nwp500/unit_system.py b/src/nwp500/unit_system.py index 093a1f9..3babef8 100644 --- a/src/nwp500/unit_system.py +++ b/src/nwp500/unit_system.py @@ -128,5 +128,5 @@ def is_metric_preferred( if unit_system is not None: return unit_system == "metric" - # If neither override nor context is set, return None (auto-detect) - return None # type: ignore[return-value] + # If neither override nor context is set, default to Fahrenheit (US) + return False diff --git a/tests/test_model_converters.py b/tests/test_model_converters.py index cfb940e..706718d 100644 --- a/tests/test_model_converters.py +++ b/tests/test_model_converters.py @@ -183,7 +183,7 @@ class TestDiv10Converter: """Test div_10 converter (divide by 10). Used for fields that need precision of 0.1 units. - Only divides numeric types (int, float), returns float(value) for others. + Divides all input types (converts to float first if needed). """ def test_zero(self): @@ -207,10 +207,9 @@ def test_float_input(self): assert div_10(50.5) == pytest.approx(5.05) def test_string_numeric(self): - """String '100' is converted to float without division.""" - # div_10 converts non-numeric to float but doesn't divide + """String '100' is converted to float and divided.""" result = div_10("100") - assert result == pytest.approx(100.0) + assert result == pytest.approx(10.0) def test_large_value(self): """1000 / 10 = 100.0.""" @@ -248,7 +247,7 @@ class TestMul10Converter: Used for energy capacity fields where the device reports in 10Wh units, but we want to store standard Wh. - Only multiplies numeric types (int, float), returns float(value) for others. + Multiplies all input types (converts to float first if needed). """ def test_zero(self): @@ -272,10 +271,9 @@ def test_float_input(self): assert mul_10(50.5) == 505.0 def test_string_numeric(self): - """String '100' is converted to float without multiplication.""" - # mul_10 converts non-numeric to float but doesn't multiply + """String '100' is converted to float and multiplied.""" result = mul_10("100") - assert result == pytest.approx(100.0) + assert result == pytest.approx(1000.0) def test_energy_capacity_example(self): """Test with realistic energy capacity values from issue #70."""