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

Merged
merged 3 commits into from
Feb 19, 2025

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 13360441514

Details

  • 62 of 91 (68.13%) changed or added relevant lines in 9 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.09%) to 79.041%

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 13 7.69%
Totals Coverage Status
Change from base Build 13355590759: -0.09%
Covered Lines: 4039
Relevant Lines: 5110

💛 - 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.

@DanGould DanGould force-pushed the into-url branch 2 times, most recently from 6d96be5 to a0c8d74 Compare February 1, 2025 17:07
@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

@DanGould DanGould requested a review from spacebear21 February 16, 2025 15:11
@DanGould DanGould marked this pull request as ready for review February 16, 2025 15:11
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.
Copy link
Collaborator

@spacebear21 spacebear21 left a comment

Choose a reason for hiding this comment

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

ACK b3f0966

@DanGould DanGould merged commit 4ba2d8c into payjoin:master Feb 19, 2025
6 checks passed
@DanGould DanGould deleted the into-url branch February 19, 2025 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants