chore: comments and test text describe only the code next to them - #75
Merged
Conversation
Reworded comments in the trigger engine, the notification helper, the provider helper, the frame coordinator, the basic book-item pool, the tile view model and three market connectors so that each describes the code next to it. Test headers and assertion messages in the trigger-engine, reconnection-engine and order-book guard tests now state the behaviour under test in plain words. The three connector project files drop an unused assembly-visibility entry. The stray README.md.bak is removed. No behaviour change: comments, strings, project metadata and one dead file.
There was a problem hiding this comment.
🟢 Approval recommended
The diff is comment/test-text cleanup plus removal of unused project metadata and a stray file, with no functional changes introduced.
Pull request overview
This PR focuses on comment and test-text hygiene across core engines, connectors, and pools, aiming to ensure inline documentation and assertion messages describe only the code they accompany (without external spec/ticket references), plus removing a stray backup file.
Changes:
- Simplified/rewrote comments in several runtime components (trigger engine, notification manager, provider staleness checks, pooling, WPF frame coordination, and multiple connectors) to describe local behavior only.
- Reworded test headers and assertion messages to state the behavior under test in plain terms.
- Removed unused
InternalsVisibleToentries from three connector.csprojfiles and deletedREADME.md.bak.
File summaries
| File | Description |
|---|---|
| VisualHFT.Plugins/MarketConnectors.Kraken/MarketConnectors.Kraken.csproj | Removes unused InternalsVisibleTo entry for a non-existent test assembly. |
| VisualHFT.Plugins/MarketConnectors.Kraken/KrakenPlugin.cs | Comment-only edits to better describe local threading/ordering/teardown behavior without external references. |
| VisualHFT.Plugins/MarketConnectors.Coinbase/MarketConnectors.Coinbase.csproj | Removes unused InternalsVisibleTo entry. |
| VisualHFT.Plugins/MarketConnectors.Coinbase/CoinbasePlugin.cs | Comment-only clarification of socket teardown severity rationale. |
| VisualHFT.Plugins/MarketConnectors.Bitfinex/MarketConnectors.Bitfinex.csproj | Removes unused InternalsVisibleTo entry. |
| VisualHFT.Plugins/MarketConnectors.Bitfinex/BitfinexPlugin.cs | Comment-only clarification of socket teardown severity rationale. |
| VisualHFT.Commons/Pools/BookItemL2Pool.cs | Updates pool documentation/comments to match the local code paths and routing. |
| VisualHFT.Commons/PluginManager/BasePluginDataRetriever.cs | Comment tweak to describe current time-source usage and shared behavior more locally. |
| VisualHFT.Commons/Helpers/HelperProvider.cs | Comment tweak to describe staleness check behavior without external references. |
| VisualHFT.Commons/Helpers/HelperNotificationManager.cs | Comment tweak on INFO tier intent/ordinal stability without external references. |
| VisualHFT.Commons.WPF/Helper/FrameCoordinator.cs | Comment tweak clarifying background-thread dispose scenario without replay-specific references. |
| ViewModel/vmTile.cs | Removes an inaccurate/out-of-scope comment line (“ONLY FOR L3 VERSION”). |
| TriggerEngine/TriggerEngineService.cs | Comment-only rewrites to describe the event and windowing/cooldown logic locally. |
| tests/Unit/VisualHFT.Commons.Tests/VisualHFT.Commons.Tests.csproj | Comment-only simplification of the test serialization rationale. |
| tests/Unit/VisualHFT.Commons.Tests/OrderBookDisposedGuardTests.cs | Rewords test commentary/assertion messages to focus on behavior rather than external call-site references. |
| tests/Integration/VisualHFT.TriggerService.Tests/TriggerEngineTests.cs | Rewords test “Act” comments to describe behavior under test plainly. |
| tests/Integration/VisualHFT.TriggerService.Tests/OnTriggerFiredEventTests.cs | Rewords test header docs/assertion messages and temp-path naming to remove external references. |
| tests/Integration/VisualHFT.TriggerService.Tests/FirstFireAndWindowedConditionTests.cs | Rewords test-file header and assertion messages to describe the two behaviors under test plainly. |
| tests/Integration/VisualHFT.DataRetriever.Tests/ReconnectionEngineTests.cs | Rewords comments describing the “declined start” outcome without referencing specific plugin files. |
| README.md.bak | Removes stray backup file from the repo. |
Review details
- Files reviewed: 20/20 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
What
Comment and test-text hygiene, no behaviour change.
README.md.bakis removed.Verification
Builds:
VisualHFT.Commons,MarketConnectors.Coinbase,MarketConnectors.Bitfinex,MarketConnectors.Kraken,VisualHFT— 0 errors.Tests:
VisualHFT.Commons.Tests25/25,VisualHFT.DataRetriever.Tests58/58,VisualHFT.TriggerService.Tests26/26 when run individually (the project's batch run is order-dependent onmasteras well; unchanged here).