Fix #16 (connecting to HN)

[tedwardd]

As outlined in #16, the current state of the code in HEAD can not successfully connect to hackernews. After some digging I discovered that the value of sys.config["proxy"] was nil causing the type assertion to a string to fail. I'm not sure that this is the BEST way to fix this issue but to the best of my knowledge, there is nowhere that appends a proxy key to sys.config{} (most systems are setting sys.config to *cfg in SetConfig() which does not, at the time it is called, have any reference to the proxy field in the config file).

I think it would be better to add this, or something similar, to SysConfig{} so that it does not need to be explicitly included in each system SetConfig() method, but attempts to do so failed, again, due to my limited knowledge around interfaces, specifically map[string]interface{}.

[greenfoo]

I have just discovered this repo and found the same issue as you :)

From what I can see, New() --> Load() (the sequence of functions called to create a new object to manage the connection to hackernews which is causing the error) can be called from two different contexts:

1. When running "neonmodem connect --type hackernews" (to add hackernews as one of the sources of data)

2. When running "neonmodem" (to navigate hackernews and other configured sources)

In the second case, first the configuration file (~/.config/neonmodem.toml) is parsed from where the value of "proxy" is obtained, then this value is copied to sys.config["proxy"] and finally New() is called from where we reach the Load() function.

In the first case none of this happens (ie. no configuration file is parsed) and thus when we reach New() --> Load() and try to access sys.config["proxy"] we get the panic: interface conversion error.

Because in this context we don't really need the proxy value for anything (the code is accessing it because the execution path is common for both contexts, but it serves no real purpose here) we can "ignore" this error somehow and keep executing.

One option is to do what you suggest in this pull request, but if you do that you will also be overwriting the value for the other context. In other words:

Context 1                            Context 2
================================================================================

                                     sys.config["proxy"] = CFG.Proxy 

New()                                New()
    sys = ...                            sys = ...                        
    sys.SetConfig()                      sys.SetConfig()
        sys.config["proxy"] = ...            sys.config["proxy"] = ...  <==== New code
    sys.load()                           sys.load()

It seems cleaner (or, at least, more "symmetrical") to emulate what is being done in Context 2, like this:

Context 1                                          Context 2
================================================================================

sys.config["proxy"] = ""   <==== New code          sys.config["proxy"] = CFG.Proxy 

New()                                              New()
    sys = ...                                          sys = ...                        
    sys.SetConfig()                                    sys.SetConfig()
    sys.load()                                         sys.load()

Or, in other words, apply this patch to the original source code:

diff --git a/cmd/connect.go b/cmd/connect.go
index 39f037a..fa73460 100644
--- a/cmd/connect.go
+++ b/cmd/connect.go
@@ -34,6 +34,7 @@ func connectBase() *cobra.Command {
        },
        Run: func(cmd *cobra.Command, args []string) {
            sysConfig = make(map[string]interface{})
+           sysConfig["proxy"] = ""
            sys, err := system.New(sysType, &sysConfig, LOG)
            if err != nil {
                LOG.Panicln(err)

WARNING: I have no previous experience with this code base, thus please don't pay too much attention to what I just said which could be completely wrong :)

[mrusme]

@tedwardd @greenfoo thanks folks, you're awesome! I'd agree with @greenfoo that his suggestion is cleaner. :) Feel free to update the PR and I'll happily merge it.

[tedwardd]

I completely agree as well. Looking at the code again in the context from @greenfoo it makes perfect sense. I was too stuck on trying to make the method being executed contain the fix that I failed to look "up" and see that there were two separate paths in cmd at play here. I've updated the PR to reflect the suggested approach.