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.

Sign up for GitHub

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

Jump to bottom

Look into setting alternative Listenbrainz hosts for a session #2

Closed
shymega opened this issue Aug 29, 2021 · 8 comments · Fixed by #6
Closed

Look into setting alternative Listenbrainz hosts for a session #2

shymega opened this issue Aug 29, 2021 · 8 comments · Fixed by #6
Assignees
@shymega
Labels
enhancement New feature or request

Comments

@shymega
Copy link
Collaborator

shymega commented Aug 29, 2021

As the title says...
@InputUsername
Copy link
Owner

I think we can add an API URL field to raw::client::Client and create a Client::with_url (or similar) constructor function to configure it. Client::new can then use that and pass the default API URL.

@shymega
Copy link
Collaborator Author

shymega commented Aug 30, 2021 •
edited

Got it working... I think. Pushing my branch for evaluation - do you want me to open a PR? Need to adjust unit tests, and add additional examples. Other than that, looks like a clean fix ...

@shymega
Copy link
Collaborator Author

shymega commented Aug 30, 2021

OK, I can confirm the fix works examples - it works either if you use Client::new() or Client::new_with_url(url: &str). I'm pleased with the fix. The only things that need fixing up now are: unit tests and examples - we should include tests for both unofficial Listenbrainz hosts (if any exist?), and official hosts. The same goes with examples.

@shymega
Copy link
Collaborator Author

shymega commented Aug 30, 2021

Having said that - we don't seem to have unit tests?

@InputUsername
Copy link
Owner

Perfect! Yeah, you can open a PR.

@InputUsername InputUsername mentioned this issue Aug 30, 2021
Open
36 tasks
@InputUsername
Copy link
Owner

Having said that - we don't seem to have unit tests?

Correct, there are currently no unit tests. Definitely something that needs to be added. I've made an issue (#5)

shymega added a commit that referenced this issue Aug 30, 2021
@shymega
Add support for alternative Listenbrainz hosts
e0d2581 
This commit adds support for alternative Listenbrainz hosts.

L10 defines the most used official host for Listenbrainz.

I have refactored the Client struct to have the `api_root_url` field.
This is automatically set to the value L10 defines, when a Client is
instantiated with `new()`. On the invocation of `client_with_url(url:
&str)`, the `api_root_url` field is set to the Listenbrainz instance
API root URL passed to the function.

The 1:1 functions <-> API methods have been modified to use
`self.api_root_url` as the base of the `format!()` macro invoked, rather
than the global constant of `API_ROOT_URL`.

This commit has been tested with the examples, and appears to be
functional.

Fixes #2.

Signed-off-by: Dom Rodriguez <shymega@shymega.org.uk>
@shymega
Copy link
Collaborator Author

shymega commented Aug 30, 2021

Pushed to the feature/alternative-hosts branch. Opening PR now...

@shymega
Copy link
Collaborator Author

shymega commented Aug 30, 2021

Having said that - we don't seem to have unit tests?

Correct, there are currently no unit tests. Definitely something that needs to be added. I've made an issue (#5)

Noted. Looking at issue #5 now.

@shymega shymega mentioned this issue Aug 30, 2021
Merged
@shymega shymega self-assigned this Aug 30, 2021
@shymega shymega added the enhancement New feature or request label Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@shymega shymega

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add support for alternative Listenbrainz hosts InputUsername/listenbrainz-rs
2 participants
@shymega @InputUsername