Skip to content

Centralize error handling and prune dead null-checks#9

Open
clementfavre wants to merge 1 commit into
viwodev:mainfrom
clementfavre:fix/dead-code-and-error-handling-refactor
Open

Centralize error handling and prune dead null-checks#9
clementfavre wants to merge 1 commit into
viwodev:mainfrom
clementfavre:fix/dead-code-and-error-handling-refactor

Conversation

@clementfavre
Copy link
Copy Markdown
Collaborator

Round 2 after #8, nothing functional this time, just cleanup so the lib is easier to maintain

There were two helper methods sitting in SaferPayClient that nobody was calling, so every request method ended up repeating the same error-handling block. I rewired everything through them pulled a couple of small helpers out, and the file shrinks a lot
On the Channels/ side I also dropped the null checks on _client since the field is readonly and always set in the ctor, which made every method a one-liner. Plus a tiny empty file in Helpers/ that nobody actually used

I'm leaving this as a draft on purpose, because I'd rather we see if we can keep things clean and make them more reliable before releasing a new version with every change, so that the next one is spotless

We should also talk a bit about what's next. I'm not exactly sure how the Saferpay documentation aligns with the project, or what parts of the API still need to be implemented :)

@viwodev
Copy link
Copy Markdown
Owner

viwodev commented May 18, 2026

Thanks again for the cleanup work and the thoughtful approach here :)

I’ll review and test the draft carefully, and most likely include it in the next update after a bit more validation. I really appreciate the effort and ideas around improving the maintainability and reliability of the project.

And of course, any further feedback, suggestions or contributions are always very welcome. Happy to improve the project together

@clementfavre
Copy link
Copy Markdown
Collaborator Author

Thanks :)
It's normal, I use it on several of my projects, so I figured I should at least contribute

The Management API is missing the terminal payment methods endpoint => https://saferpay.github.io/jsonapi/#rest_customers_customerId_terminals_terminalId_payment-methods

It seems to me that the rest looks fine, otherwise, maybe just a couple of small things to clarify on the side.
SaferPayMethods.cs has constants for QueryPaymentMeans and AdjustAmount but no wrappers, and I couldn't find them in the current v1.52 spec, not sure if they're leftovers from an older version or stubs that never made it in

I'll open separate PRs for the terminal payment methods endpoint and a few of the smaller cleanup ideas I mentioned, so we can keep things easy to review one at a time
And you don't need to bump the version with each PR, is that okay with you if you do a "big" release for the next one ?

@clementfavre
Copy link
Copy Markdown
Collaborator Author

if that works for you, I'll create issues outlining what I see as improvements to the project, and a pull request will be linked to the issue
Generally, I'll handle the issues I create, we'll see what you'd like to implement yourself

@clementfavre clementfavre marked this pull request as ready for review May 19, 2026 06:38
@viwodev
Copy link
Copy Markdown
Owner

viwodev commented May 20, 2026

Hi @D0LBA3B

I really appreciate all the contributions and feedback, thank you :)

I originally built and used this library for one of my own projects, but I’m not actively using Saferpay at the moment anymore, so lately I’ve mainly been trying to keep the library aligned with the official documentation. Because of that, there will probably always be small gaps or missing pieces here and there.

Having support from someone actively using the API is honestly very valuable, and I think with your contributions the library can become much more solid and useful over time. Thanks again.

@clementfavre
Copy link
Copy Markdown
Collaborator Author

@viwodev Thanks, I really appreciate it ☺️
Honestly it's already cool that you keep the lib aligned with the docs even when you're not using Saferpay yourself, plenty of maintainers would have just let it root

I'll keep pushing stuff as I hit it on my side, makes sense to share back since it benefits both of us
Quick thing on the workflow side, could you bump my permissions a bit so I can assign people on issues ? Mainly so I can self-assign when I take something on, and link PRs to the issues they close

@viwodev
Copy link
Copy Markdown
Owner

viwodev commented May 20, 2026

Absolutely 🙂 I’ve added you as a collaborator.

Since you’re actively using the library, I think your feedback and contributions will help the project improve much faster and in a much more practical direction. Really appreciate all the help and effort so far. Thanks!

@clementfavre clementfavre requested a review from viwodev May 28, 2026 16:22
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