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

Move query to doRequest function parameter #81

Merged
merged 1 commit into from
Apr 4, 2023

Conversation

tardypad
Copy link

@tardypad tardypad commented Apr 3, 2023

There were bugs in some functions where the "?" character was missing in the getPath call. Now the addition of it is centralised in the doRequest function.

This also fix the getPath naming. It is expected to only return the path of the URL and not the query part.

There were bugs in some functions where the "?" character was missing in
the getPath call. Now the addition of it is centralised in the doRequest
function.

This also fix the getPath naming. It is expected to only return the path
of the URL and not the query part.
@majidkarimizadeh
Copy link
Collaborator

majidkarimizadeh commented Apr 4, 2023

I don't think passing a query string as an argument would be a good idea! and you can see in most cases you pass empty string!

I would suggest the following solution:


// rest.go
type Path struct {
    endpoint  string
    query       string
}

func doRequest(ctx context.Context, method string, path Path, args ...interface{}) error {}

the name of Path can be something else :)

@tardypad
Copy link
Author

tardypad commented Apr 4, 2023

type Path struct {
endpoint string
query string
}

At this point, it might be better to try to use https://pkg.go.dev/net/url#URL directly but that's also a bigger refactor.
Can we maybe do it in a separate PR so as not to block @msslr ?

@majidkarimizadeh
Copy link
Collaborator

I created an issue for this! As I mentioned, passing empty string in most cases is not the thing that what we want but for not blocking @msslr, I'll merge it!

#82

@majidkarimizadeh majidkarimizadeh merged commit 40ed474 into Leaseweb:master Apr 4, 2023
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