Skip to content

Propagate errors when retrieving exp parameter#441

Merged
DanGould merged 2 commits intopayjoin:masterfrom
shinghim:399-propagate-exp
Dec 14, 2024
Merged

Propagate errors when retrieving exp parameter#441
DanGould merged 2 commits intopayjoin:masterfrom
shinghim:399-propagate-exp

Conversation

@shinghim
Copy link
Contributor

Follow-up on #428 to propagate errors when retrieving the exp parameter. This is the last of the parameter getters that need an update to return a Result, so this will close #399

@shinghim shinghim marked this pull request as ready for review December 13, 2024 04:05
@coveralls
Copy link
Collaborator

coveralls commented Dec 13, 2024

Pull Request Test Coverage Report for Build 12324387724

Details

  • 32 of 60 (53.33%) changed or added relevant lines in 2 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.04%) to 67.045%

Changes Missing Coverage Covered Lines Changed/Added Lines %
payjoin/src/uri/url_ext.rs 30 58 51.72%
Files with Coverage Reduction New Missed Lines %
payjoin/src/send/mod.rs 2 92.77%
Totals Coverage Status
Change from base Build 12280546564: -0.04%
Covered Lines: 3131
Relevant Lines: 4670

💛 - Coveralls

Copy link
Contributor

@DanGould DanGould left a comment

Choose a reason for hiding this comment

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

Another banger by @shinghim ! On a roll 💥

Other than some minor suggestions, one thing that might make sense is to move the uri_ext-specific errors into that file since they require a feature gate otherwise, and they're not generally applicable to the uri module. Perhaps a follow up commit to the change could move those feature-gated errors & their related impls into? url_ext.rs

@shinghim
Copy link
Contributor Author

@DanGould yes i think moving those errors to url_ext.rs makes sense. I'll update this with the minor changes + moving the errors

@shinghim shinghim requested a review from DanGould December 13, 2024 14:56
@shinghim shinghim force-pushed the 399-propagate-exp branch 2 times, most recently from 1c01ae3 to c34f620 Compare December 13, 2024 14:58
Copy link
Collaborator

@nothingmuch nothingmuch left a comment

Choose a reason for hiding this comment

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

i'm confused by the test, but i think i'm just being an idiot? if i am then please explain, if i'm not then arguably that's an unrelated bug that i introduced and should be fixed separately, so utACK either way

Copy link
Contributor

@DanGould DanGould left a comment

Choose a reason for hiding this comment

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

ACK 4ea00db

@DanGould DanGould merged commit e8f1069 into payjoin:master Dec 14, 2024
6 checks passed
DanGould added a commit to DanGould/rust-payjoin that referenced this pull request Jan 3, 2025
Summary:

Take the payjoin-cli out of alpha now that the wire protocol is presumed
to be stable.

Changelog:

- Bump payjoin to v0.22.0 with stable wire protocol
  - Allow mixed input scripts in v2 (payjoin#367)

- Fix bug to propagate missing  config parameter or argument error (payjoin#441)
- Don't pause between long polling requests (payjoin#463)
- Hide danger-local-https feature with _ prefix (payjoin#423)
This was referenced Jan 3, 2025
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.

url_ext fragment parameter ParseErrors should be propagated

4 participants