Skip to content

Default to application/xml but Accept both#111

Merged
twogood merged 1 commit intomainfrom
xml-headers
Oct 20, 2025
Merged

Default to application/xml but Accept both#111
twogood merged 1 commit intomainfrom
xml-headers

Conversation

@twogood
Copy link
Owner

@twogood twogood commented Oct 20, 2025

Summary by CodeRabbit

  • Bug Fixes

    • Improved XML media type handling to support both "application/xml" and "text/xml" formats with standardized content type management.
  • Tests

    • Enhanced XML error response test coverage to validate multiple media type scenarios.

@twogood twogood self-assigned this Oct 20, 2025
@coderabbitai
Copy link

coderabbitai bot commented Oct 20, 2025

Walkthrough

The pull request converts an XML error response test to a parameterized Theory supporting multiple media types and updates the XML builder to dynamically configure Accept headers from supported media types while standardizing ContentType to "application/xml".

Changes

Cohort / File(s) Summary
Test Parameterization
Activout.RestClient.Xml.Test/ErrorResponseXmlTest.cs
Converts TestErrorResponse_Xml_BadRequest() from a single Fact to a parameterized Theory with InlineData for media types "application/xml" and "text/xml"; adds string mediaType parameter and uses it in mocked response instead of hard-coded "text/xml"
Builder Configuration
Activout.RestClient.Xml/RestClientBuilderXmlExtensions.cs
Replaces hard-coded Accept("text/xml") and ContentType("text/xml") with dynamic Accept header generation from XmlHelper.SupportedMediaTypes and fixed ContentType("application/xml")

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • Add tests for ErrorResponse without JSON #108: Directly modifies the same ErrorResponseXmlTest behavior and alters XML media type configuration in the builder extensions with matching test parameterization and content-type standardization.

Poem

🐰 XML media types, now dancing free—
No more hard-coded lines for thee!
From "text/xml" to what we need,
Supported types in tests we seed!
Flexible config, tests parameterized—
A rabbit's builder optimized! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "Default to application/xml but Accept both" accurately captures the primary objective of the changeset. The modifications across both files demonstrate exactly this intent: RestClientBuilderXmlExtensions.cs is updated to set ContentType to "application/xml" (the default) while allowing Accept to include multiple media types from XmlHelper.SupportedMediaTypes, and ErrorResponseXmlTest.cs is parameterized to verify this behavior works with both "application/xml" and "text/xml". The title is concise, specific, and clear enough that a teammate scanning the repository history would immediately understand this PR addresses XML media type handling.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch xml-headers

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1913dfe and ed4273a.

📒 Files selected for processing (2)
  • Activout.RestClient.Xml.Test/ErrorResponseXmlTest.cs (2 hunks)
  • Activout.RestClient.Xml/RestClientBuilderXmlExtensions.cs (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.cs: Use [] list syntax for collections
Use file-scoped namespaces
Use primary constructors wherever possible
Use records for data transfer objects (DTOs) and immutable data structures
Use var for local variable declarations when possible
Use expression-bodied members for simple methods and properties
Use pattern matching for type checks and deconstruction
Use using statements for resource management
Use async and await for asynchronous programming
Never include "Async" in method names
Only include comments that explain why something is done, not what is done

Files:

  • Activout.RestClient.Xml/RestClientBuilderXmlExtensions.cs
  • Activout.RestClient.Xml.Test/ErrorResponseXmlTest.cs
🧬 Code graph analysis (1)
Activout.RestClient.Xml/RestClientBuilderXmlExtensions.cs (1)
Activout.RestClient.Xml/XmlHelper.cs (1)
  • XmlHelper (3-10)
🔇 Additional comments (3)
Activout.RestClient.Xml/RestClientBuilderXmlExtensions.cs (1)

10-11: LGTM! Solid implementation of the Accept/ContentType strategy.

The dynamic Accept header generation from XmlHelper.SupportedMediaTypes maintains a single source of truth, and standardizing on "application/xml" for ContentType aligns with modern XML standards while still accepting both media types in responses.

Activout.RestClient.Xml.Test/ErrorResponseXmlTest.cs (2)

27-30: LGTM! Excellent test coverage improvement.

Converting to a parameterized Theory validates that error response deserialization works correctly for both supported XML media types, ensuring the builder's Accept header changes function as expected.


42-42: LGTM! Clean integration of the media type parameter.

The parameterized media type properly exercises the mock response, ensuring both "application/xml" and "text/xml" are validated in the test flow.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@twogood twogood merged commit 905af8c into main Oct 20, 2025
9 checks passed
@twogood twogood deleted the xml-headers branch October 20, 2025 18:29
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.

1 participant