Skip to content

Conversation

davidhewitt
Copy link
Contributor

@davidhewitt davidhewitt commented Sep 16, 2025

Change Summary

My take on #1719.

I am reasonably confident that the issue only applies to empty paths, so in this variation I named the config option preserve_empty_path (default False for backwards compatibility).

Related issue number

pydantic/pydantic#7186

Checklist

  • Unit tests for the changes exist
  • Documentation reflects the changes where applicable
  • Pydantic tests pass with this pydantic-core (except for expected changes)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Copy link

codspeed-hq bot commented Sep 16, 2025

CodSpeed Performance Report

Merging #1789 will not alter performance

Comparing dh/url-preserve-empty-path (63c6d6e) with main (cbe2dd2)

Summary

✅ 163 untouched

@davidhewitt davidhewitt marked this pull request as ready for review September 18, 2025 13:07
@Viicos Viicos changed the title add option to preserve empty URL paths Add option to preserve empty URL paths Sep 29, 2025
Copy link
Member

@Viicos Viicos left a comment

Choose a reason for hiding this comment

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

LGTM!

}

pub fn __str__(&self) -> String {
pub fn __str__(&self, py: Python<'_>) -> String {
Copy link
Member

Choose a reason for hiding this comment

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

If str(...) is called from Python, I assume this doesn't error because of a missing argument (same for other methods)?

let vios = RefCell::new(None);

let url = Url::options()
// if we're in strict mode, we collect consider a syntax violation as an error
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// if we're in strict mode, we collect consider a syntax violation as an error
// if we're in strict mode, we collect considering a syntax violation as an error

?

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.

2 participants