Skip to content

Add HTTPS/domain support via url crate, bump edition/version, add CI and tests#4

Merged
haydnv merged 4 commits into
mainfrom
codex/add-https-and-domain-name-support-czm2xv
May 6, 2026
Merged

Add HTTPS/domain support via url crate, bump edition/version, add CI and tests#4
haydnv merged 4 commits into
mainfrom
codex/add-https-and-domain-name-support-czm2xv

Conversation

@code-tc

@code-tc code-tc commented May 6, 2026

Copy link
Copy Markdown
Contributor

Motivation

  • Support HTTPS URLs, domain names (including IDNA) and offload host parsing/normalization to a dedicated URL parser instead of custom ad-hoc parsing.
  • Ensure correctness across feature combinations by adding a CI job matrix that runs the test suite for common feature sets.
  • Modernize the crate metadata and dependencies (edition bump and dependency version updates) to align with maintained crates.

Description

  • Added a GitHub Actions workflow at .github/workflows/ci.yml to run cargo test across a feature matrix including --no-default-features, default, --all-features, and individual features hash, stream, serialize, and uuid.
  • Reworked host parsing to delegate parsing and IDNA/domain normalization to the url crate by introducing use url::Url; and parsing Host from a Url::parse result, and added rejection checks for userinfo, query, fragment, and unexpected path components.
  • Introduced Protocol::HTTPS and Address::Domain(String) variants and updated ordering, as_ip, is_localhost, and GetSize implementations accordingly.
  • Bumped crate version to 0.5.0, edition to 2024, updated dependency versions (including destream = "0.10", hr-id = "0.7", added url = "2.5") and added serde_json in [dev-dependencies].
  • Added unit tests exercising host/link parsing, IDNA normalization and rejection cases in src/lib.rs, serde roundtrip tests in src/serial.rs, and destream trait presence tests in src/stream.rs, and a minor import reordering in src/path.rs.

Testing

  • Added unit tests in src/lib.rs, src/serial.rs, and src/stream.rs and ran the test suite with cargo test, and all tests passed locally.
  • Added a CI workflow to run cargo test ${{ matrix.args }} for multiple feature combinations so the same unit tests will be executed automatically on push and pull requests.

Codex Task

@chatgpt-codex-connector

Copy link
Copy Markdown

💡 Codex Review

pathlink/src/lib.rs

Lines 318 to 319 in 862b02c

let url =
Url::parse(s).map_err(|cause| ParseError::from(format!("invalid host: {cause}")))?;

P1 Badge Preserve IPv6 round-trip compatibility in host parsing

Switching Host::from_str to Url::parse makes IPv6 parsing require bracketed literals (e.g. http://[::1]), but Host/Link formatting still emits unbracketed IPv6 hosts (http://::1...). As a result, values containing IPv6 that were previously parseable—and strings produced by this crate’s own Display impls—can now fail to parse, breaking round-trip behavior and existing inputs that relied on the prior format.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/lib.rs
Comment on lines +959 to +978
#[test]
fn parse_link_accepts_uppercase_https() {
let link: Link = "HTTPS://EXAMPLE.COM/a/b".parse().expect("link");
assert_eq!(link.to_string(), "https://example.com/a/b");
}

#[test]
fn parse_link_accepts_ipv6_host() {
let link: Link = "https://[::1]:443/a/b".parse().expect("link");
assert_eq!(link.to_string(), "https://[::1]:443/a/b");
}

#[test]
fn parse_link_rejects_unsupported_protocol() {
let err = "ftp://example.com/a"
.parse::<Link>()
.expect_err("unsupported protocol should fail");
assert!(err.to_string().contains("invalid protocol"));
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we are missing edge case test here. if we try parse link from "https://example.com". Result link via to_string() will produce "https://example.com/" with / at the end. Because url crate sets the PathBuf to root /. Is it expected behaviour?

@haydnv
haydnv merged commit 73bb99f into main May 6, 2026
7 checks passed
@haydnv
haydnv deleted the codex/add-https-and-domain-name-support-czm2xv branch May 6, 2026 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants