Skip to content

chore: pre-release polish (filename, versioned user-agent, SD parser pragmas)#227

Merged
tylerkron merged 2 commits into
mainfrom
chore/pre-release-polish-223
May 24, 2026
Merged

chore: pre-release polish (filename, versioned user-agent, SD parser pragmas)#227
tylerkron merged 2 commits into
mainfrom
chore/pre-release-polish-223

Conversation

@tylerkron
Copy link
Copy Markdown
Contributor

Summary

Three small, independent cleanups bundled together per #223:

  • Rename TestMessage.csTextMessage.cs so the filename matches the production class TextMessage inside. No other source-tree callers exist.
  • Versioned user-agent in GitHubFirmwareDownloadService — derive from Assembly.GetName().Version so GitHub rate-limit/abuse fingerprints follow releases instead of staying pinned at 1.0. Required flipping conststatic readonly because the interpolation isn't compile-time.
  • SD parser async shims — replaced the no-op await Task.CompletedTask; (with weak // keep the method async-compatible comment) with #pragma warning disable CS1998 around the iterator methods. These need async to allow yield return but have no real awaits; the pragma makes that intent explicit and survives TreatWarningsAsErrors. Applied to the three production parser methods (ProduceSamples, ParseJsonLines, ParseCsvLines) and also the two EmptySamples() helpers in Json/Csv parsers for in-file consistency.

Closes #223

Test plan

  • dotnet build clean on net9.0 + net10.0 with TreatWarningsAsErrors enforced
  • dotnet test — 995 passed, 0 failed (2 hardware-skipped) on both targets
  • CI green

🤖 Generated with Claude Code

…pragmas)

Three small cleanups bundled together:

1. Rename TestMessage.cs to TextMessage.cs to match its class — the file
   contained `class TextMessage` (a production message type), not test code.

2. Derive GitHubFirmwareDownloadService user-agent from the assembly version
   instead of a hardcoded "1.0", so GitHub rate-limit/abuse fingerprints move
   with each release. `const` becomes `static readonly` since the
   interpolation can't be evaluated at compile time.

3. Replace the no-op `await Task.CompletedTask;` shim in the SD parser
   IAsyncEnumerable iterators with `#pragma warning disable CS1998`. These
   methods need `async` to allow `yield return`, but have no real awaits;
   the pragma documents that intent explicitly and survives
   TreatWarningsAsErrors. Applied to the three production parser methods
   and the two `EmptySamples()` helpers in Json/Csv parsers for consistency.

Closes #223

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@tylerkron tylerkron requested a review from a team as a code owner May 23, 2026 23:53
@qodo-code-review
Copy link
Copy Markdown

Review Summary by Qodo

Pre-release polish: pragmas, versioned user-agent, filename fix

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Replace no-op await Task.CompletedTask with #pragma warning disable CS1998 in async iterators
  - Applied to three production parser methods and two EmptySamples() helpers
  - Makes async requirement explicit and survives TreatWarningsAsErrors
• Derive user-agent version from assembly instead of hardcoded "1.0"
  - Ensures GitHub rate-limit fingerprints follow releases
  - Changed const to static readonly for runtime interpolation
• Rename TestMessage.cs to TextMessage.cs for filename accuracy
Diagram
flowchart LR
  A["SD Parser Iterators"] -->|"Replace await shim"| B["Pragma disable CS1998"]
  C["GitHub Download Service"] -->|"Runtime version"| D["Dynamic user-agent"]
  E["Test file"] -->|"Rename"| F["TextMessage.cs"]
  B --> G["Cleaner intent"]
  D --> G
  F --> G

Loading

File Changes

1. src/Daqifi.Core/Device/SdCard/SdCardCsvFileParser.cs ✨ Enhancement +4/-3

Replace async shim with CS1998 pragma

• Added #pragma warning disable CS1998 before ParseCsvLines() method
• Removed await Task.CompletedTask no-op statement
• Added #pragma warning restore CS1998 after method body
• Applied same pragma pattern to EmptySamples() helper method

src/Daqifi.Core/Device/SdCard/SdCardCsvFileParser.cs


2. src/Daqifi.Core/Device/SdCard/SdCardFileParser.cs ✨ Enhancement +2/-2

Replace async shim with CS1998 pragma

• Added #pragma warning disable CS1998 before ProduceSamples() method
• Removed await Task.CompletedTask no-op statement
• Added #pragma warning restore CS1998 after method body

src/Daqifi.Core/Device/SdCard/SdCardFileParser.cs


3. src/Daqifi.Core/Device/SdCard/SdCardJsonFileParser.cs ✨ Enhancement +4/-3

Replace async shim with CS1998 pragma

• Added #pragma warning disable CS1998 before ParseJsonLines() method
• Removed await Task.CompletedTask no-op statement
• Added #pragma warning restore CS1998 after method body
• Applied same pragma pattern to EmptySamples() helper method

src/Daqifi.Core/Device/SdCard/SdCardJsonFileParser.cs


View more (2)
4. src/Daqifi.Core/Firmware/GitHubFirmwareDownloadService.cs ✨ Enhancement +2/-1

Derive user-agent version from assembly

• Changed DEFAULT_USER_AGENT from const to static readonly
• Updated user-agent string to derive version from assembly at runtime
• Uses typeof(GitHubFirmwareDownloadService).Assembly.GetName().Version for dynamic versioning

src/Daqifi.Core/Firmware/GitHubFirmwareDownloadService.cs


5. src/Daqifi.Core/Communication/Messages/TextMessage.cs Additional files +0/-0

...

src/Daqifi.Core/Communication/Messages/TextMessage.cs


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented May 23, 2026

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider


Remediation recommended

1. User-agent loses prerelease suffix ✓ Resolved 🐞 Bug ◔ Observability
Description
GitHubFirmwareDownloadService builds the user-agent from Assembly.GetName().Version, which is
numeric-only and cannot include prerelease identifiers (e.g. "-beta.1"), so prerelease packages
can’t be uniquely identified in GitHub API traffic. This conflicts with the repo’s release workflow,
which explicitly permits prerelease SemVer tags and uses that value for package versioning.
Code

src/Daqifi.Core/Firmware/GitHubFirmwareDownloadService.cs[R11-12]

Evidence
The service sets its User-Agent using Assembly.GetName().Version, while the release workflow
accepts prerelease SemVer tags and uses them for build/package versioning; numeric-only Version
cannot carry the prerelease suffix, so those builds won’t be uniquely identifiable via the
user-agent string.

src/Daqifi.Core/Firmware/GitHubFirmwareDownloadService.cs[9-47]
.github/workflows/release.yml[25-46]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`DEFAULT_USER_AGENT` uses `Assembly.GetName().Version`, which cannot represent prerelease SemVer suffixes (e.g. `-beta.1`). The repo’s release workflow allows such versions and uses them for package versioning, so GitHub API traffic from prerelease builds becomes harder to distinguish.

### Issue Context
- Release workflow allows prerelease SemVer (`1.0.0-beta.1`) and uses it as the version input.
- Current user-agent source is `AssemblyName.Version`.

### Fix Focus Areas
- src/Daqifi.Core/Firmware/GitHubFirmwareDownloadService.cs[11-12]

### Suggested fix
Derive the version from `AssemblyInformationalVersionAttribute` (fallback to `AssemblyName.Version` if missing), e.g.:

- Read `AssemblyInformationalVersionAttribute.InformationalVersion`
- Sanitize if needed to ensure it’s a valid `User-Agent` product version token
- Fall back to `Assembly.GetName().Version?.ToString()` if informational version isn’t present

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

…mVer suffix

Per Qodo review on #227: `Assembly.GetName().Version` is numeric-only
("1.0.0.0") and truncates SemVer prerelease suffixes. The release workflow
passes `/p:Version=1.0.0-beta.1` which sets `AssemblyInformationalVersion`
but writes only `1.0.0.0` to `AssemblyVersion`, so prerelease builds would
have been indistinguishable from stable ones in GitHub API traffic.

Switch to `AssemblyInformationalVersionAttribute.InformationalVersion` with
`AssemblyName.Version` as a fallback. Both are RFC 7231 user-agent token
compliant (the `+` SourceLink suffix is allowed in product-version).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@tylerkron
Copy link
Copy Markdown
Contributor Author

Response to @qodo-code-review

1. User-agent loses prerelease suffix — Agreed, fixed in de53f6f

Good catch. The release workflow passes /p:Version=${VERSION} where VERSION can be a SemVer prerelease like 1.0.0-beta.1. That sets AssemblyInformationalVersion to 1.0.0-beta.1 but truncates AssemblyVersion to 1.0.0.0 — so prerelease builds would have been indistinguishable in GitHub API traffic, defeating the whole purpose of versioning the user-agent.

Switched to AssemblyInformationalVersionAttribute.InformationalVersion with AssemblyName.Version?.ToString() as fallback (and "unknown" if both are somehow null). Both forms are RFC 7231 product-version compliant — the + SourceLink commit suffix is allowed in the token grammar, so no sanitization needed.

private static string BuildDefaultUserAgent()
{
    var assembly = typeof(GitHubFirmwareDownloadService).Assembly;
    var version = assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>()?.InformationalVersion
                  ?? assembly.GetName().Version?.ToString()
                  ?? "unknown";
    return $"DaqifiFirmwareUpdater/{version}";
}

All 995 tests still pass on net9.0 and net10.0.

@tylerkron tylerkron merged commit 52d3ffa into main May 24, 2026
1 check passed
@tylerkron tylerkron deleted the chore/pre-release-polish-223 branch May 24, 2026 00:00
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.

chore: small pre-release polish (filename, user-agent versioning, SD parser async shims)

1 participant