-
Notifications
You must be signed in to change notification settings - Fork 0
Address review feedback on MIME detection and sync tool #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Address review feedback on MIME detection and sync tool #2
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors MIME detection and introduces a sync tool to improve performance and data freshness. The changes replace allocation-heavy ZIP-based content sniffing with efficient span-based ASCII comparisons and implement reusable data structures for parsing mime.types listings.
Key changes:
- Replaced dictionary-based MIME sniffing with span-based ASCII pattern matching to avoid string allocations
- Added comprehensive content detection with magic signature support for common file types
- Introduced sync tool for updating MIME database from upstream sources with curated overrides
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updated documentation with comprehensive feature overview, usage examples, and installation instructions |
| ManagedCode.MimeTypes/mimeTypes.json | Added new MIME mappings and reordered entries for consistency |
| ManagedCode.MimeTypes/MimeTypeCategory.cs | Added new categories for Script, Binary, Multipart, and Message |
| ManagedCode.MimeTypes/MimeHelper.cs | Major refactor with span-based content detection, reverse lookup API, and runtime registration |
| ManagedCode.MimeTypes/ManagedCode.MimeTypes.csproj | Updated LangVersion to preview |
| ManagedCode.MimeTypes.sln | Added MimeTypes.Sync project reference |
| ManagedCode.MimeTypes.Tests/ | Added comprehensive test coverage for new functionality |
| ManagedCode.MimeTypes.Sync/ | New console utility for synchronizing MIME database |
| ManagedCode.MimeTypes.Generator/MimeTypeSourceGenerator.cs | Enhanced generator with proper escaping and improved key parsing |
| .github/workflows/mime-sync.yml | Added automated workflow for weekly MIME database updates |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| continue; | ||
| } | ||
|
|
||
| var parts = line.Split(MimeTypeSeparators, StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries); |
Copilot
AI
Oct 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MimeTypeSeparators array is allocated on each call to Split(). Consider using line.Split(' ', '\t', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries) directly or make the array static readonly at the class level to avoid repeated allocations.
| var parts = line.Split(MimeTypeSeparators, StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries); | |
| var parts = line.Split(' ', '\t', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries); |
ManagedCode.MimeTypes/MimeHelper.cs
Outdated
| yield break; | ||
| } | ||
|
|
||
| var separatorIndex = trimmed.IndexOfAny(new[] { '?', '#' }); |
Copilot
AI
Oct 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The char array new[] { '?', '#' } is allocated on each call. Consider making this a static readonly field to avoid repeated allocations.
| using System.Text; | ||
| using System.Text.RegularExpressions; | ||
| using System.Threading; | ||
|
|
Copilot
AI
Oct 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition header.Length >= 12 is checked twice in succession. The inner check on line 573 is redundant since it's already verified by the outer condition on line 572.
| using System.Text; | ||
| using System.Text.RegularExpressions; | ||
| using System.Threading; | ||
|
|
Copilot
AI
Oct 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the previous issue, the condition header.Length >= 12 is checked twice. The inner check on line 594 is redundant since it's already verified by the outer condition on line 592.
| const string extension = "customext"; | ||
| const string mime = "application/x-custom"; | ||
|
|
||
| try | ||
| { | ||
| MimeHelper.RegisterMimeType(extension, mime); | ||
| MimeHelper.GetMimeType($"file.{extension}").ShouldBe(mime); | ||
|
|
||
| MimeHelper.TryGetExtensions(mime, out var extensions).ShouldBeTrue(); | ||
| extensions.ShouldContain($".{extension}"); | ||
| } | ||
| finally | ||
| { | ||
| MimeHelper.UnregisterMimeType(extension).ShouldBeTrue(); | ||
| MimeHelper.GetMimeType(extension).ShouldBe("application/octet-stream"); |
Copilot
AI
Oct 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Using const for these values is unnecessary since they're only used within this single method. Consider using var or regular string literals directly in the method calls for better readability.
| const string extension = "customext"; | |
| const string mime = "application/x-custom"; | |
| try | |
| { | |
| MimeHelper.RegisterMimeType(extension, mime); | |
| MimeHelper.GetMimeType($"file.{extension}").ShouldBe(mime); | |
| MimeHelper.TryGetExtensions(mime, out var extensions).ShouldBeTrue(); | |
| extensions.ShouldContain($".{extension}"); | |
| } | |
| finally | |
| { | |
| MimeHelper.UnregisterMimeType(extension).ShouldBeTrue(); | |
| MimeHelper.GetMimeType(extension).ShouldBe("application/octet-stream"); | |
| try | |
| { | |
| MimeHelper.RegisterMimeType("customext", "application/x-custom"); | |
| MimeHelper.GetMimeType($"file.{"customext"}").ShouldBe("application/x-custom"); | |
| MimeHelper.TryGetExtensions("application/x-custom", out var extensions).ShouldBeTrue(); | |
| extensions.ShouldContain($".{"customext"}"); | |
| } | |
| finally | |
| { | |
| MimeHelper.UnregisterMimeType("customext").ShouldBeTrue(); | |
| MimeHelper.GetMimeType("customext").ShouldBe("application/octet-stream"); |
Pull Request Test Coverage Report for Build 18431903005Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
@codex fix all comments |
|
Summary
Testing
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Restore ArrayPool import
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68ea08250b3483269ee0369c9166f2aa