🐛 fix: Subdomains offset handling #3495
Conversation
WalkthroughThe logic of the Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant DefaultCtx
Test->>DefaultCtx: Subdomains(offset)
alt offset < 0 or offset ≥ subdomain count
DefaultCtx-->>Test: []
else if offset == 0
DefaultCtx-->>Test: all subdomain parts
else
DefaultCtx-->>Test: subdomain parts excluding last offset labels
end
Suggested labels
Suggested reviewers
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
gaby
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR fixes subdomain offset handling, ensuring that when an offset exceeds the available subdomains the method returns an empty slice. The changes include updating the test expectations in ctx_test.go and modifying the Subdomains logic in ctx.go to avoid slice bounds errors.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| ctx_test.go | Updated tests to reflect the corrected subdomain handling. |
| ctx.go | Modified Subdomains to return an empty slice when the offset makes the slice value non-positive. |
Comments suppressed due to low confidence (1)
ctx.go:1760
- When the host contains only a single segment (e.g. 'localhost'), the current logic does not return an empty slice even though the new behavior expects it. Consider adding a condition (e.g. if len(subdomains) <= 1 { return [] }) before calculating the offset to ensure consistent subdomain handling.
l := len(subdomains) - o
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3495 +/- ##
==========================================
- Coverage 83.87% 83.84% -0.04%
==========================================
Files 120 120
Lines 12272 12278 +6
==========================================
+ Hits 10293 10294 +1
- Misses 1555 1559 +4
- Partials 424 425 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@gaby check markdown lint |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
docs/api/ctx.md (3)
1347-1352: Nit: unify “sub-domain” terminology
The term “sub-domain” is hyphenated here, but elsewhere the code and docs use “subdomain” as one word. For consistency and to avoid confusion, consider changing “sub-domain” → “subdomain.”🧰 Tools
🪛 LanguageTool
[misspelling] ~1347-~1347: This word is normally spelled as one.
Context: ...omains Returns a slice with the host’s sub-domain labels. The dot-separated parts that pr...(EN_COMPOUNDS_SUB_DOMAIN)
1363-1372: Enhance examples to cover edge cases
Great interactive example. To fully illustrate the updated logic, add:
- A case for
offset > count(e.g.,c.Subdomains(10) // [])- An example with a port in the host (e.g.,
Host: "tobi.ferrets.example.com:8080" → c.Subdomains() // ["tobi","ferrets"])
This will help users understand both the port‐stripping and out‐of‐range behavior.
1600-1602: Nit: clarify default filename handling
It’s worth noting that the defaultfilename=parameter uses the basename of the given path (not the full path). Consider updating to:
“By default, thefilename=parameter is set to the file’s basename (e.g.,report-12345.pdf), not the full path.”
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ctx.go(1 hunks)ctx_test.go(1 hunks)docs/api/ctx.md(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ctx.go
- ctx_test.go
🧰 Additional context used
🪛 LanguageTool
docs/api/ctx.md
[misspelling] ~1347-~1347: This word is normally spelled as one.
Context: ...omains Returns a slice with the host’s sub-domain labels. The dot-separated parts that pr...
(EN_COMPOUNDS_SUB_DOMAIN)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/api/ctx.md (1)
1347-1347: Correct compound spelling.“sub-domain” should be one word: “subdomain”.
Apply:- Returns a slice with the host’s sub-domain labels. + Returns a slice with the host’s subdomain labels.🧰 Tools
🪛 LanguageTool
[misspelling] ~1347-~1347: This word is normally spelled as one.
Context: ...omains Returns a slice with the host’s sub-domain labels. The dot-separated parts that pr...(EN_COMPOUNDS_SUB_DOMAIN)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/api/ctx.md(2 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/api/ctx.md
[misspelling] ~1347-~1347: This word is normally spelled as one.
Context: ...omains Returns a slice with the host’s sub-domain labels. The dot-separated parts that pr...
(EN_COMPOUNDS_SUB_DOMAIN)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Compare
- GitHub Check: repeated
- GitHub Check: unit (1.24.x, macos-13)
- GitHub Check: unit (1.24.x, windows-latest)
🔇 Additional comments (3)
docs/api/ctx.md (3)
1349-1354: Description and signature update looks good.The default offset explanation and the new variadic
offset ...intsignature correctly reflect the implementation.
1356-1363: Subdomain offset behavior table is comprehensive.The table now clearly covers omitted (default), valid ranges, boundary and invalid cases.
1370-1373: Examples accurately reflect new behavior.The sample outputs for default, positive, zero and negative offsets match the implementation.
Summary