Fix write files error #47 #50

asdf · 2019-10-08T07:17:57Z

asdf commented

2019-10-08 07:17:57 +00:00

This is a fix for #47.

File paths are being constructed by concatenating strings, and was missing a forward slash.

This is a fix for #47. File paths are being constructed by concatenating strings, and was missing a forward slash.

asdf self-assigned this 2019-10-08 07:17:57 +00:00

sloum was assigned by asdf

2019-10-08 07:17:57 +00:00

sloum requested changes 2019-10-08 14:09:09 +00:00

sloum left a comment

This was a really good find! Exposes I bug I wouldnt have thought of. I left a note for futher action.

client.go Outdated

						
				@ -507,3 +504,3 @@

						return

					}

					savePath := c.Options["savelocation"] + name

					savePath := c.Options["savelocation"] + "/" + name

sloum commented

2019-10-08 14:08:22 +00:00

This is a good find, however, I think we'll need to check for the presence of a terminal slash on savelocation. I have one, so saving was working for me (which is why this was a surprise). So I think doing:

if len(c.Options["savelocation"]) > 0 && c.Options["savelocation"][len(c.Options["savelocation"]) - 1] == "/" {
  savePath := c.Options["savelocation"] + name
} else {
  savePath := c.Options["savelocation"] + "/" + name
}

There may be a more elegant way to go about it. I think there is a strings.HasSuffix function that might be easier.

This is a good find, however, I think we'll need to check for the presence of a terminal slash on savelocation. I have one, so saving was working for me (which is why this was a surprise). So I think doing: ``` if len(c.Options["savelocation"]) > 0 && c.Options["savelocation"][len(c.Options["savelocation"]) - 1] == "/" { savePath := c.Options["savelocation"] + name } else { savePath := c.Options["savelocation"] + "/" + name } ``` There may be a more elegant way to go about it. I think there is a `strings.HasSuffix` function that might be easier.

asdf commented

2019-10-08 22:07:49 +00:00

The path/filepath library has a Join function to construct a path in each of the cases suggested. It is also compatible with multiple operating systems, so it should work on Windows too.

savePath := filepath.Join(c.Options["savelocation"], name)

What do you think of this approach?

Also it seems the same issue exists in function saveFileFromData

The path/filepath library has a Join function to construct a path in each of the cases suggested. It is also compatible with multiple operating systems, so it should work on Windows too. savePath := filepath.Join(c.Options["savelocation"], name) What do you think of this approach? Also it seems the same issue exists in function `saveFileFromData`

sloum commented

2019-10-09 00:59:08 +00:00

Awesome! Really good find. Lets go with that. It checks off a lot of good boxes: standard library, few to no edge cases, cross platform.

Can you add it in both places? If you dont have time I can pull this branch and do so, just let me know.

Awesome! Really good find. Lets go with that. It checks off a lot of good boxes: standard library, few to no edge cases, cross platform. Can you add it in both places? If you dont have time I can pull this branch and do so, just let me know.

sloum added the

bug

label 2019-10-09 02:30:36 +00:00

asdf commented

2019-10-09 07:52:05 +00:00

saveFile and saveFileFromData now use path/filepath to join savelocation and filename.

I've tested and can download gopher and gemini documents. I'm not sure how saveFileFromData is called exactly, so haven't tested that.

One thing I noticed during this fix is there are other path operations like:
nameSplit := strings.Split(u.Resource, "/")
filename := nameSplit[len(nameSplit)-1]
path/filepath handles files paths on multiple operating systems, and the path library focuses on forward-slash only paths like URLs. It has functions like Base to return the last element of a path, which could replace the above with:
filename := path.Base(u.Resource)
If you are interested in making this change, let me know.

saveFile and saveFileFromData now use `path/filepath` to join savelocation and filename. I've tested and can download gopher and gemini documents. I'm not sure how saveFileFromData is called exactly, so haven't tested that. One thing I noticed during this fix is there are other path operations like: nameSplit := strings.Split(u.Resource, "/") filename := nameSplit[len(nameSplit)-1] `path/filepath` handles files paths on multiple operating systems, and the `path` library focuses on forward-slash only paths like URLs. It has functions like [Base](https://golang.org/pkg/path/#Base) to return the last element of a path, which could replace the above with: filename := path.Base(u.Resource) If you are interested in making this change, let me know.

sloum commented

2019-10-09 15:44:34 +00:00

Yes. I think it would be good to replace my hacked together versions of these things with cross-platform support by the standard library where possible. I think this PR solves the issue as presented for writing files so I will merge this in and open an issue for combing through the codebase and making adjustments to path oriented operations.

sloum closed this pull request

2019-10-09 15:44:39 +00:00

Sign in to join this conversation.

No reviewers