sloum/bombadillo
sloum
/
bombadillo
5
27
Fork 7
Code Issues 18 Pull Requests 1 Releases 9 Activity

Makes local protocol navigable #94

Manually merged
sloum merged 3 commits from improved-local into develop 2019-11-17 23:05:25 +00:00
Conversation 7 Commits 3 Files Changed 3 +26 -12
2 changed files with 26 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 3ee7bd9c7b - Show all commits

4
client.go
View File

@ -963,13 +963,13 @@ func (c *client) handleTelnet(u Url) {
}
func (c *client) handleLocal(u Url) {
content, err := local.Open(u.Resource)
content, links, err := local.Open(u.Resource)
if err != nil {
c.SetMessage(err.Error(), true)
c.DrawMessage()
return
}
pg := MakePage(u, content, []string{})
pg := MakePage(u, content, links)
pg.WrapContent(c.Width - 1)
c.PageState.Add(pg)
c.SetPercentRead()

34
local/local.go
View File

@ -4,39 +4,53 @@ import (
"fmt"
"io/ioutil"
"os"
"path/filepath"
"sort"
"strings"
)
func Open(address string) (string, error) {
func Open(address string) (string, []string, error) {
links := make([]string, 0, 10)
if !pathExists(address) {
return "", fmt.Errorf("Invalid system path: %s", address)
return "", links, fmt.Errorf("Invalid system path: %s", address)
}
file, err := os.Open(address)
if err != nil {
return "", fmt.Errorf("Unable to open file: %s", address)
return "", links, fmt.Errorf("Unable to open file: %s", address)
}
defer file.Close()
if pathIsDir(address) {
fileList, err := file.Readdirnames(0)
fileList, err := file.Readdir(0)
if err != nil {
return "", fmt.Errorf("Unable to read from directory: %s", address)
return "", links, fmt.Errorf("Unable to read from directory: %s", address)
}
asdf commented 2019-11-17 20:07:20 +00:00
Outdated
Review
Copy Link

I've found it can be useful to include the error message coming from the FS operation. It will normally indicate why something has happened, and the error message is somewhat readable. There's other instances of this here, is this something you would consider changing?

I've found it can be useful to include the error message coming from the FS operation. It will normally indicate why something has happened, and the error message is somewhat readable. There's other instances of this here, is this something you would consider changing?
sloum commented 2019-11-17 22:59:44 +00:00
Review
Copy Link

I agree. The error messaging for this module is generally pretty readable and will be useful to users. I have changed this in the relevant locations in locals.go.

I agree. The error messaging for this module is generally pretty readable and will be useful to users. I have changed this in the relevant locations in `locals.go`.
var out strings.Builder
out.WriteString(fmt.Sprintf("Current directory: %s\n\n", address))
for _, obj := range fileList {
out.WriteString(obj)
sort.Slice(fileList, func(i, j int) bool {
return fileList[i].Name() < fileList[j].Name()
asdf commented 2019-11-17 20:27:28 +00:00
Outdated
Review
Copy Link

Looking at bash, it includes ".." in the list of files in root, so this distinction may not be needed from a user perspective. The behaviour I see is that following .. from root just lands you back at root. Removing this logic check seems to do the same thing with no error messages, maybe it can be omitted?

If you did want to keep doing this, hardcoding root as "/" may not be as compatible as using filepath.VolumeName (but, I don't really know, it is just the closest thing I could see).

Looking at bash, it includes ".." in the list of files in root, so this distinction may not be needed from a user perspective. The behaviour I see is that following `..` from root just lands you back at root. Removing this logic check seems to do the same thing with no error messages, maybe it can be omitted? If you did want to keep doing this, hardcoding root as `"/"` may not be as compatible as using `filepath.VolumeName` (but, I don't really know, it is just the closest thing I could see).
sloum commented 2019-11-17 23:01:24 +00:00
Review
Copy Link

Yup, a quick test in play.golang.org shows that this works well. I agree ditching that hard-coded base path is a better cross-platform solution. I have removed the if and it just always builds the .. path version. I also added some basic comments to the code in-case it ever needs revisiting.

Yup, a quick test in `play.golang.org` shows that this works well. I agree ditching that hard-coded base path is a better cross-platform solution. I have removed the `if` and it just always builds the `..` path version. I also added some basic comments to the code in-case it ever needs revisiting.
})
for i, obj := range fileList {
linkNum := fmt.Sprintf("[%d]", i+1)
out.WriteString(fmt.Sprintf("%-5s ", linkNum))
out.WriteString(fmt.Sprintf("%s ", obj.Mode().String()))
out.WriteString(obj.Name())
out.WriteString("\n")
fp := filepath.Join(address, obj.Name())
links = append(links, fp)
}
return out.String(), nil
return out.String(), links, nil
}
bytes, err := ioutil.ReadAll(file)
if err != nil {
return "", fmt.Errorf("Unable to read file: %s", address)
return "", links ,fmt.Errorf("Unable to read file: %s", address)
}
return string(bytes), nil
return string(bytes), links, nil
}
func pathExists(p string) bool {