Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use IntoUrl for more ergonomic function signatures #520

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

DanGould
Copy link
Contributor

@DanGould DanGould commented Jan 30, 2025

Re: #513
Inspired by reqwest::IntoUrl

After some consideration, I'm not sure if the goal really should be to remove url types from the public API. That crate is ancient and has a stable release cadence and conservative MSRV targets (1.63 even with the latest) that we can track.

However, I still think the IntoUrl interface makes our typestate machines easier to work downstream with for the tradeoff of added complexity in our crate.

In order to graduate from DRAFT I'd need new error types for extract functions that include an into_url::Error encapsulating variant. Before I do that work, I'm seeking a concept ack from @spacebear21 or otherwise a rejection of the idea, presumably on the grounds of too much complexity.

Note, in payjoin-cli We're still validating URLs in configuration, and using the url::Url abstraction in function signatures makes more sense than becoming more loose for testing.

The greatest advantage of this change may accrue to downstream FFI users, who I imagine use their native language's Url type and conversion to a stringly type that payjoin-ffi handles using IntoUrl rather than forcing them to use the payjoin::Url re-export and error handling.

@DanGould DanGould changed the title Into url Use IntoUrl for more ergonomic function signatures Jan 30, 2025
@DanGould DanGould added enhancement New feature or request api labels Jan 30, 2025
@coveralls
Copy link
Collaborator

coveralls commented Jan 30, 2025

Pull Request Test Coverage Report for Build 13090676415

Details

  • 56 of 86 (65.12%) changed or added relevant lines in 7 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.1%) to 78.428%

Changes Missing Coverage Covered Lines Changed/Added Lines %
payjoin/src/send/v2/error.rs 0 3 0.0%
payjoin/src/io.rs 4 9 44.44%
payjoin/src/into_url.rs 27 36 75.0%
payjoin/src/receive/v2/error.rs 1 14 7.14%
Files with Coverage Reduction New Missed Lines %
payjoin/src/send/mod.rs 2 93.91%
Totals Coverage Status
Change from base Build 13090176852: -0.1%
Covered Lines: 3683
Relevant Lines: 4696

💛 - Coveralls

@spacebear21
Copy link
Collaborator

cACK, I don't think the complexity is too wild, and I agree 100% with this point:

This reduces a step for [downstream implementations] and unifies our error handling for added
internal complexity, which seems like the express purpose of the library.

Linking to reqwest::IntoUrl in the module-level documentation or even the commit message would help clearly point to the source of inspiration for our approach.

Comment on lines 9 to 18
impl std::fmt::Display for Error {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
Error::BadScheme => write!(f, "URL scheme is not allowed"),
Error::ParseError(e) => write!(f, "{}", e),
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

A neat trick I saw in rust-bitcoin to avoid repetition:

Suggested change
impl std::fmt::Display for Error {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
Error::BadScheme => write!(f, "URL scheme is not allowed"),
Error::ParseError(e) => write!(f, "{}", e),
}
}
}
impl std::fmt::Display for Error {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
use Error::*;
match self {
BadScheme => write!(f, "URL scheme is not allowed"),
ParseError(e) => write!(f, "{}", e),
}
}
}

@DanGould DanGould force-pushed the into-url branch 2 times, most recently from 6d96be5 to a0c8d74 Compare February 1, 2025 17:07
Accept stringly types in  typestate methods so that downstream
implementations may depend on the typestate machines for URL parsing.

This reduces a step for them and unifies our error handling for added
internal complexity, which seems like the express purpose of the library.

Create new ParseUrl error types in our state machines.
Removed gate is already inside a v2 feature module.
`use InternalSessionError::*;` is DRY.
@DanGould
Copy link
Contributor Author

DanGould commented Feb 1, 2025

Rather than link to the reqwest IntoUrl docs at the module level, I made the type pub (since it's in function signatures) and documented the inspiration in the type's docstring. If it were documented at the private module level there'd be no way to find it.

@DanGould
Copy link
Contributor Author

DanGould commented Feb 5, 2025

Blocking this until #522 is addressed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api blocked enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants