Address PR17/PR18 review comments - #19
Merged
Merged
Conversation
- Bus.cs: detect last station by index, not station type (fixes bypassed cancellation handling when a station type is reused); add regression test - README: correct target-framework references to netstandard2.0, fix WithAbortOnCancel example to assign bus and read bus.Package.IsAborted, drop stale 'Last updated' date - publish.yml: pin dotnet/nbgv to a release SHA (v0.5.2); add concurrency guard so publish runs serialize
There was a problem hiding this comment.
Pull request overview
This PR addresses prior review feedback from PRs #17/#18 by fixing a cancellation edge case in the bus pipeline (last-station detection), updating README guidance to match the netstandard2.0 targeting and cancellation usage, and hardening the publish workflow to avoid concurrent release races.
Changes:
- Fix
Bus.GoAsynclast-station detection to use the current index (prevents mis-detecting “last” when a station type is reused). - Add a regression test that reproduces the reused-station-type cancellation scenario.
- Update docs and CI publish workflow:
netstandard2.0wording, cancellation example adjustments, serialize publish runs, and pindotnet/nbgvto a SHA.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| README.md | Updates targeting/cancellation documentation and examples for netstandard2.0 + AbortOnCancel. |
| Cargo/Bus.cs | Corrects last-station detection logic to be index-based. |
| Cargo.Tests/Integration/Async.cs | Adds regression coverage for reused station-type with final station + cancellation. |
| .github/workflows/publish.yml | Serializes publish runs and pins dotnet/nbgv to a specific commit SHA. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+231
to
+232
| // No exception thrown — check the package state instead: | ||
| bus.Package.IsAborted // true |
Address PR19 review: replace bare 'bus.Package.IsAborted' expression with an if-statement so the snippet compiles.
Pick up the CI workflow so this PR is gated before merge.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the Copilot review comments from #17 and #18.
Code
currentStationIndex == stationList.Count - 1) instead of station-type equality. The old check misfired when a station type was reused, bypassing cancellation handling. Adds a regression test (fails on the old code, passes now).Docs (README)
netstandard2.0WithAbortOnCancel()example so it compiles (assignsbus, readsbus.Package.IsAborted)Workflow
dotnet/nbgvto a release SHA (v0.5.2)concurrencyguard so publish runs serialize (closes the Auto-create GitHub Release on publish #18 race comment)Tests: 45/45 on net8.0.