Add optional addressable-normalize to Twingly::URL::Extended - #169
Conversation
Eric-Twingly
changed the title
|
@bjornkellner Renaming the branch auto closed the original, tried to address your comments from #167 in 6653091. |
There was a problem hiding this comment.
🟡 Changes recommended
The public struct layout breaks positional consumers, and invalid URLs reject the new normalization keyword.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds optional Addressable canonicalization before extended URL normalization and hashing.
Changes:
- Adds
addressable_normalizesupport and exposes the canonicalized URL. - Expands tests and documents the new behavior.
- Identified compatibility issues with
HashResultordering and invalid URLs.
File summaries
| File | Description |
|---|---|
lib/twingly/url/extended.rb |
Implements Addressable normalization and extends hash results. |
spec/lib/twingly/url/extended_spec.rb |
Tests canonicalization and hashing behavior. |
README.md |
Documents the new API and result field. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| original_url = twingly_url.original_url_without_blacklisted_parameters | ||
| original_url = twingly_url.original_url_without_blacklisted_parameters | ||
| addressable_normalized = twingly_url.addressable_normalized |
There was a problem hiding this comment.
I consider addressable_normalized_url the new "standard url" that we will use and pass through our system, i think it should have the blacklisted parameters removed.
Same in the addressable_normalized function. Maybe add it there, even though the name get's a bit misguiding.
There was a problem hiding this comment.
Added blacklisted parameter stripping to the addressable normalized URL in 10c9129. I opted to not include the blacklisted parameter stripping in the addressable_normalized function because it would be misleading, like you suggested.
bjornkellner
left a comment
bjornkellner
left a comment
There was a problem hiding this comment.
Nice job!
maybe update version ?
Optionally percent-encode URLs.
Refactors to avoid parsing multiple times, and renames to addressable normalize to more accurately reflect the normalization applied.
Renames the addressable normalized URL in normalize_and_calculate_urlhash return to callable URL, which includes blacklist stripping.
10c9129 to
41ab605
Compare
Rename of #167.
Adds an opt-in
addressable normalizeto extended normalization.