Add DefaultShell as required by #69 #106

contrun · 2018-07-13T09:56:57Z

Added a DefaultShell function to return a default shell and error as required by issue 69.

Stebalien · 2018-07-13T10:13:41Z

shell.go

+	urls = append(urls, "/ip4/127.0.0.1/tcp/5001", "https://ipfs.io")
+
+	// do not repeat encountered addresses
+	encountered := map[string]bool{}


The idiom is to just use map[string]struct{} and check for presence.

Thank you for your advise. I will revise it to use map[string]struct{}.

Stebalien · 2018-07-13T10:15:34Z

shell.go

@@ -84,6 +84,42 @@ func NewShellWithClient(url string, c *gohttp.Client) *Shell {
 	}
 }

+// Get shell from environmental variables, default api path or gateway.
+func DefaultShell() (*Shell, error) {


It would be nice to use a sync.Once and cache the default client. Thoughts?

Will DefaultShell be called more than once? I think people will normally call it for one shot.

I can see people calling DefaultShell().DoThing() quite often.

Stebalien · 2018-07-13T10:16:03Z

shell.go

@@ -84,6 +84,42 @@ func NewShellWithClient(url string, c *gohttp.Client) *Shell {
 	}
 }

+// Get shell from environmental variables, default api path or gateway.


We should spell out exactly how we pick the default shell.

What do you think if I add some functions which respectively obtain new shells from environmental variables, default gateway and the api file. I think the order of shells to try in the function DefautlShell is sensible, as normally the environmental variables are explicitly set by the user and I have tried to use gateway as the the last resort.

Sounds good to me.

Stebalien · 2018-07-13T10:16:24Z

Thanks!

add functions to obtain new shells from environmental variables, default gateway and the default api file.

Stebalien

I'd prefer to:

Reduce the API surface. That is, don't expose anything we don't actually need to expose.
Reuse NewLocalShell where possible. NewLocalShell should probably be modified to use IPFS_API if set.

After thinking about it for a bit, we should probably be a little less smart. That is,

If IPFS_API is set, use it regardless.
If an appropriate API file exists, use it regardless.
Otherwise, use ipfs.io.

This way:

The behavior is completely unsurprising.
We don't need to perform any checks. We could still leave a constructor that checks the API endpoint but we wouldn't need one.

I'd rather error out than pick the wrong API endpoint (e.g., if my local one is down).

Stebalien · 2018-07-16T15:50:22Z

shell.go

+func DefaultShell() *Shell {
+	// Load once and cache working default shell address
+	defaultShellAddrLoadOnce.Do(defaultShell)
+	return NewShell(defaultShellAddr)


I'd just reuse the same shell. Its thread-safe and, really, we want to reuse the http client.

Also, that'd get rid of the need for DefaultShellWithCheck.

If the apifile does exist, but an error occurred while reading the apifile, should we just return nil? This will also be surprising, as people will see unexpected panic. In this extreme case, should we just call panic?

Hm. Yeah, that's unfortunate. We shouldn't panic. We could:

Return nil.

Return an error (stashing it somewhere). That makes the API a bit more difficult to use but that's probably not that much of an issue.

Return a special shell that always fails? Probably a bad idea.

I'd prefer 2.

Do you mind clarify where should I put the error?

Put it in a variable, guarded by a sync.Once.

I did not add a sync.Once to reading api file. For the time being, reading api file will only be called by NewLocalShell, which is already guarded by a sync.Once. Sorry for the delay. I had a really busy week on some earthly bussiness.

No need to apologize for delays, there's no rush on this.

Stebalien · 2018-07-16T15:59:05Z

shell.go

+			}
+		}
+	}
+	return nil, errors.New(fmt.Sprintf("No working server in %v", urls))


I'd just return nil instead of an error (the error isn't very useful).

Stebalien · 2018-07-16T16:01:04Z

shell.go

@@ -84,6 +92,94 @@ func NewShellWithClient(url string, c *gohttp.Client) *Shell {
 	}
 }

+// Load all shell urls with error checking
+func NewShellWithCheck(urls ...string) (*Shell, error) {


Maybe TryNewShell? Not happy with the name but can't think of a better one.

Anyways, this should probably have some more documentation (the description doesn't explain what "error checking" means and would otherwise lead me to believe that it returns a slice of shells).

Try to reuse NewLocalShell to return a local Shell add DefaultShell to return a shell from default api address add test functions to NewLocalShell, DefaultShell and TryNewShell

…aultshell

Stebalien · 2018-08-21T22:14:36Z

Sorry to drag this out... I'm a bit worried that the current naming will confuse users:

NewLocalShell returns a reused, potentially non-local shell.
DefaultShell doesn't check the API file, env, etc.

I'd have two functions:

LocalShell that does everything the current LocalShell does except that it doesn't check the gateway.
DefaultShell that calls LocalShell and, if no local shell exists (we can check for a special ErrNoShell error), it falls back on a gateway shell.

Thoughts?

If you're short on time, I can make these changes myself. I hate changing direction on people like this.

contrun · 2018-08-28T12:02:33Z

I think we have different perspectives. For me, all I have to do is adding something that works, while you will be maintaining the code for a long time. You gotta be especailly careful when maintaining a public api. I understand your points. I think they are very valid, and I really appreciate your comments. Unfortunately, currently I do not have much time. I may have sometime this weekend. If you have time currently, please feel free to change it.

Add DefaultShell as required by ipfs#69

22e2ede

Stebalien reviewed Jul 13, 2018

View reviewed changes

Stebalien self-assigned this Jul 13, 2018

use sync.Once to obtain default shell

a48d7d7

add functions to obtain new shells from environmental variables, default gateway and the default api file.

Stebalien suggested changes Jul 16, 2018

View reviewed changes

B YI added 2 commits July 28, 2018 12:02

reuse NewLocalShell, add DefautlShell

ede2fd1

Try to reuse NewLocalShell to return a local Shell add DefaultShell to return a shell from default api address add test functions to NewLocalShell, DefaultShell and TryNewShell

Merge branch 'master' of https://github.com/ipfs/go-ipfs-api into def…

921c4e1

…aultshell

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DefaultShell as required by #69 #106

Add DefaultShell as required by #69 #106

contrun commented Jul 13, 2018

Stebalien Jul 13, 2018

contrun Jul 13, 2018

Stebalien Jul 13, 2018

contrun Jul 13, 2018

Stebalien Jul 13, 2018

Stebalien Jul 13, 2018

contrun Jul 13, 2018

Stebalien Jul 13, 2018

Stebalien commented Jul 13, 2018

Stebalien left a comment

Stebalien Jul 16, 2018

contrun Jul 17, 2018 •

edited

Loading

Stebalien Jul 18, 2018

contrun Jul 24, 2018

Stebalien Jul 25, 2018

contrun Jul 28, 2018

Stebalien Jul 30, 2018

Stebalien Jul 16, 2018

Stebalien Jul 16, 2018

Stebalien commented Aug 21, 2018

contrun commented Aug 28, 2018

Add DefaultShell as required by #69 #106

Are you sure you want to change the base?

Add DefaultShell as required by #69 #106

Conversation

contrun commented Jul 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien commented Jul 13, 2018

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

contrun Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien commented Aug 21, 2018

contrun commented Aug 28, 2018

contrun Jul 17, 2018 •

edited

Loading