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

Add custom auth URLs and simplify get/etc methods #356

Merged
merged 4 commits into from
Jan 4, 2023
Merged

Conversation

marioortizmanero
Copy link
Collaborator

@marioortizmanero marioortizmanero commented Jul 30, 2022

Description

This adds a configurable prefix URL for authentication.

I've also simplified the request methods in the base client. We don't really need get if it's only used in endpoint_get. We can also extend post_form to auth_post, which also prepends the auth URL for convenience, just like endpoint_get. Finally, I've renamed endpoint_get and similars to api_get for consistency and because it's clearer and much shorter.

Motivation and Context

See #350

Dependencies

None

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How has this been tested?

I've modified the endpoint_url test, and added another one for auth_url.

Is this change properly documented?

TODO

@marioortizmanero marioortizmanero marked this pull request as draft July 30, 2022 10:45
@marioortizmanero marioortizmanero changed the title Prefix URLs Add custom auth URLs and simplify get/etc methods Dec 29, 2022
@@ -10,6 +10,7 @@ abstract class BaseClient {
ClientResult<()> auto_reauth()
ClientResult<()> refresh_token()
String endpoint_url()
String auth_url()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ramsayleung, can you generate the diagram again? Do you know if there's a way to do this automatically instead?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, I can regenerate this diagram, but generating the diagram manually is a little clumsy, I would like to figure out how to generate this diagram automatically.

Copy link
Owner

Choose a reason for hiding this comment

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

I think we could use this GitHub Action to generate an image from plantuml file automatically: https://github.com/marketplace/actions/generate-plantuml

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great! Then you can ignore this comment, and we'll set it up in a separate PR.

@marioortizmanero marioortizmanero marked this pull request as ready for review December 29, 2022 13:26
@marioortizmanero marioortizmanero changed the base branch from master to more-chrono-duration December 29, 2022 14:22
@marioortizmanero
Copy link
Collaborator Author

Note that #375 should be merged before this.

Base automatically changed from more-chrono-duration to master January 3, 2023 06:22
@ramsayleung
Copy link
Owner

Everything looks good to me, except for the discussion about how to generate an image file from plantuml file automatically.

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