Skip to content

Conversation

@wvdvegt
Copy link

@wvdvegt wvdvegt commented Sep 28, 2020

Should fix the issue.

1) Fixed URL segment separator issue. in GetPackageByName() preventing use on Microsoft Windows.
// GetPackageByName returns a package by a given name
func (c *PackagistClient) GetPackageByName(name string) (*PackagistPackage, *http.Response, error) {
u := fmt.Sprintf("%s/packages%s.json", c.url.String(), filepath.Clean("/"+name))
u := fmt.Sprintf("%s/packages%s.json", c.url.String(), strings.Replace(filepath.Clean("/"+name), "\\", "/", -1))
Copy link
Owner

Choose a reason for hiding this comment

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

In #6 (comment) you pointed out that the issue might be the usage of filepath.Clean. What do you think about removing the filepath.Clean reference and replacing it with an URL fitting pattern? e.g. typical URL encoding?
Have a look at https://www.urlencoder.io/golang/ to check if one of those might be a good choice.

@wvdvegt
Copy link
Author

wvdvegt commented Sep 29, 2020

Hi
My experience with go is only 1 hr, so code might be improved. The main advantage of filetpath.Clean is it removes some unwanted stuff from the package name. But there might be better ways.

@wvdvegt
Copy link
Author

wvdvegt commented Oct 1, 2020

Hi

The following replacement appears to work on Windows (and using net/url):

	//u := fmt.Sprintf("%s/packages%s.json", c.url.String(), strings.Replace(filepath.Clean("/"+name), "\\", "/", -1))
	u, err := url.Parse(c.url.String())

	// Note: not sure this code is necceasry (as the url.Parse can't fail):
	//if err != nil {
	//	return nil, nil, err
	//}

	u.Path += "/packages"
	u.Path += fmt.Sprintf("/%s.json", name)

	resp, err := c.httpClient.Get(u.String())

I'm not sure the first if statement is necessary as the packagist.org URL is probably hardcoded and not modifiable using a command line switch (so the url.Parse() can't fail.

Personally I think this is indeed a better solution (properly building the url instead of stitching it together as a string)

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