OAuth open URL in default browser, set success message and throw desc…

[stevenleadbeater]

• [oauth] Open authorization URL in default browser
• [oauth] Allow optionally passing success message to display on browser return page
• [oauth] Throw specific errors on failure states

[kingosticks]

My initial thought is I should have made this an object rather than just a function, having to break the API just to support a custom response message is a shame.

[stevenleadbeater]

My initial thought is I should have made this an object rather than just a function, having to break the API just to support a custom response message is a shame.

Is it worth working this up a bit more so that there are overloads instead without too much copy paste? I don't mind doing the leg work on that. It would mean you can keep the existing API and we can add a method to pass the message

[kingosticks]

I think that sounds good.

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

[roderickvd]

@stevenleadbeater friendly nudge - would you address the comment you and @kingosticks discussed above?

[kingosticks]

There was another issue recently where they needed to do something different for android. I can't remember or find it now. My point is we could do with way to pass more options without breaking API (builder pattern maybe?).

[photovoltex]

Another option besides builder pattern that allows flexible parameters adjustment, is an <method>_with_options alternative method (like here https://github.com/librespot-org/librespot/blob/dev/core/src/spclient.rs#L435). It's a bit simpler and straight forward in comparison to the builder pattern.

[kingosticks]

Another option besides builder pattern that allows flexible parameters adjustment, is an <method>_with_options alternative method (like here https://github.com/librespot-org/librespot/blob/dev/core/src/spclient.rs#L435). It's a bit simpler and straight forward in comparison to the builder pattern.

But if you add a new arg to that, isn't that still a breaking ApI change?

[photovoltex]

If the struct with the args doesn't have Default implemented yes, otherwise you could use a struct initialization with ..Default::default() for the remaining args you don't want to use.

But even then people could initialize it directly and that would result in breaking changes... So yeah not fault proof but a very simple alternative and an easy solution to not break existing apis, but going forward not so good.

Just had the "best" case in mind.

[kingosticks]

Happy with the idea that people who program defensively against breaking changes are catered for. Sounds good, easy is good.