Skip to content

Conversation

dzuelke
Copy link
Contributor

@dzuelke dzuelke commented Sep 16, 2025

GitUrl::url_compat_display() is currently private, but it would be very useful to have this functionality - in my case, normalizing an SSH style URL is fine, especially because I do not want to bother with handling the 'url lifetime of GitUrl in my wrapper enum.

This change implements fallible conversion to url::Url for GitUrl and &GitUrl via GitUrl::url_compat_display().

Longer term it might be wise to re-visit the use of a lifetime for the purpose of re-using slices of the input string - the url::Url crate does not bother with such micro-optimizations either, presumably for good reasons.

Obviously, to use a GitUrl as a url::Url, the conversion to url::Url still has to happen twice (because GitUrl::parse() incurs the conversion via GitUrl::is_valid()), but that can be optimized in a separate change, e.g. via a new GitUrl::parse_to_url(), or by removing the conversion from GitUrl::is_valid() altogether, leaving it to the caller to attempt the conversion.

GitUrl::url_compat_display() is currently private, but it would be very useful to have this functionality - in my case, normalizing an SSH style URL is fine, especially because I do not want to bother with handling the 'url lifetime of GitUrl in my wrapper enum.

This change implements fallible conversion to url::Url for GitUrl and &GitUrl via url_compat_display().

Longer term it might be wise to re-visit the use of a lifetime for the purpose of re-using slices of the input string - the url::Url crate does not bother with such micro-optimizations either, presumably for good reasons.

Obviously, to use a GitUrl as a url::Url, the conversion to url::Url still has to happen twice (because GitUrl::parse() incurs the conversion via GitUrl::is_valid()), but that can be optimized in a separate change (e.g. via a new GitUrl::parse_to_url(), or by removing the conversion from is_valid() altogether, leaving it to the caller to attempt the conversion).
@tjtelan
Copy link
Owner

tjtelan commented Sep 16, 2025

GitUrl::parse_to_url() was originally planned, but didn't make it in this latest effort. I'll add it into the backlog.

Ideally, I'd like to enhance the overall quality of the parsing to the point where I can remove url from the default features, making it easier for users to include url only when it's more convenient for their needs.

In the meantime, converting to url::Url is intended as a temporary solution until I have more time to improve error handling during parsing, so I can provide more accurate feedback to users, which is something url::Url handles more reliably.

Longer term it might be wise to re-visit the use of a lifetime for the purpose of re-using slices of the input string - the url::Url crate does not bother with such micro-optimizations either, presumably for good reasons.

I appreciate this feedback. This is fair, and worth a review in the future.

Thanks for your time and the contribution!

@tjtelan tjtelan merged commit 28a2988 into tjtelan:main Sep 16, 2025
12 checks passed
@github-actions github-actions bot mentioned this pull request Sep 16, 2025
tjtelan pushed a commit that referenced this pull request Sep 16, 2025
## 🤖 New release

* `git-url-parse`: 0.5.1 -> 0.5.2 (✓ API compatible changes)

<details><summary><i><b>Changelog</b></i></summary><p>

<blockquote>

##
[0.5.2](v0.5.1...v0.5.2)
- 2025-09-16

### Added

- Implement TryFrom<GitUrl> for url::Url
([#66](#66))
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/release-plz/release-plz/).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
dzuelke added a commit to heroku/buildpacks-php that referenced this pull request Sep 17, 2025
Version 0.5.2 includes tjtelan/git-url-parse-rs#66 which we need. Pinning to 0.5.x for now since there might be other breaking/structural changes coming soon.

GUS-W-19637481
@dzuelke
Copy link
Contributor Author

dzuelke commented Sep 17, 2025

Thank you for the super quick merge and release, very much appreciated!

dzuelke added a commit to heroku/buildpacks-php that referenced this pull request Sep 17, 2025
Version 0.5.2 includes tjtelan/git-url-parse-rs#66 which we need. Pinning to 0.5.x for now since there might be other breaking/structural changes coming soon.

GUS-W-19637481
dzuelke added a commit to heroku/buildpacks-php that referenced this pull request Sep 17, 2025
Version 0.5.2 includes tjtelan/git-url-parse-rs#66 which we need. Pinning to 0.5.x for now since there might be other breaking/structural changes coming soon.

GUS-W-19637481
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